-
Notifications
You must be signed in to change notification settings - Fork 9.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow users to use gitignore templates in existing repositories #14943
base: development
Are you sure you want to change the base?
Allow users to use gitignore templates in existing repositories #14943
Conversation
@thenewpotato first contribution to GitHub Desktop 🎉 🎉 🎉 As an improvement, I think it's a good idea to show the template picker only when the .gitignore file is empty, as suggested in the second paragraph of #2197 (comment) |
23a2eb3
to
c597b06
Compare
@tsvetilian-ty Thanks for the feedback and for the welcome! I have pushed new changes that incorporate your feedback. The UI now uses a The PR description and screen-recorded demo have been updated. |
@thenewpotato thanks for the changes! I'm not sure if @sergiou87 what are your thoughts on this, as someone involved in the original enhancement discussion? |
@tsvetilian-ty I agree with that. I went ahead and removed the hint text and tweaked the Demo GIF in the PR description has been updated. Let me know if there's anything else! |
Hi there!! Sorry, I've been busy and missed this 😢 |
Sorry again for the delay, I finally took a look at it. It feels odd that you can select a template, make changes, and still be able to choose another template (potentially destroying any customization), that's why I suggested the hint text + something to create the gitignore from the template. But it's true I can't think of a better option than this. Maybe we should do something like… when the gitignore text is empty, we show a hint text saying something like "It looks like Once the gitignore text area is not empty, both the hint and the In order to avoid much movement of UI elements, maybe it's better to put the hint and It's still not 100% great. I totally understand why @thenewpotato initially used a context menu: because you take the direct action of selecting the template, but as @tsvetilian-ty said, using a context menu to display a list of possible values is not a common pattern. Another option would be showing that I'd like to know your thoughts about my suggestion, and also to hear what @tidy-dev thinks about this. |
I think I like direction of show and hide the select component on empty. I don't think it needs a hint if it appears and disappears on content clearing/adding. It is true tho it is kind of jumpy to do this - I can't see top vs bottom helping (unless we keep a "blank" space at bottom for this to appear in). This is playing with adding an animation to help with the jumpiness.. Additionally, we could put a "Clear to select a new ignore template" paragraph in this select place when hidden to help with jumpiness too. With "Clear" being a link, so they could clear in one click to get back to templates. Other notes with below, is need some logic to set the select back to "None" when user clears text. :) Screen.Recording.2022-07-26.at.8.44.07.AM.mov |
Other option with it below: Screen.Recording.2022-07-26.at.10.14.49.AM.mov |
Hey @thenewpotato ! I've talked to @tidy-dev and shared some ideas. We still are not 100% convinced about any of them, so we're gonna talk about this with the rest of the team when we're all back from vacations (mid-August). Sorry it's taking this long, summer is just not the best time 😅 Thank you for your patience 🙇♂️ |
@sergiou87 @tidy-dev No worries at all! Thank you for taking the time to look into this. I also noticed that the current behavior is such that you can switch the template mid-editing and overwrite changes in the textarea. I was fine with this behavior because changes in the textarea aren't final anyways--they aren't written to an actual gitignore until the user hits the submit button. If the user isn't happy with the changes, they can always click cancel and start over. In either case, let me know the result of your discussion with the team! I'm not bent on my approach and would be happy to implement any changes :) |
Hi - just wanted to make some comments as I will make great use of this and commented on #2197 . @thenewpotato great job with this PR - your screen capture video is exactly what I had in mind and is slightly simpler and better than what I proposed. Great job. @sergiou87 @tidy-dev in my opinion, it's OK for the dropdown not to disappear after making a selection. In real-world use cases, someone will not do a significant amount of work in the text area and then change the dropdown to something else. At most they would copy and paste something. When they go back in to this dialog after saving (and when there's content in the If you feel strongly that the dropdown should disappear immediately after selecting an option, and want to have it reappear if someone deletes all the content from the text area, I suppose that's fine. I think/agree it should default to Excellent work all around and many thanks. |
Hi again! Sorry for making you wait, we've been going back and forth with this a few times internally trying to come up with a trade-off that satisfies all of us. First of all, a month ago @tidy-dev introduced a new component called Screen.Recording.2022-09-22.at.11.49.55.AM.movWe thought it would be nice to go back to the original idea of having a hint with a link suggesting the use of a template "It looks like However, we think it'd be useful to keep the hint around even when the Regarding the dropdown itself, just as the one in the video above, it would be a filterable list of |
Hi all - still very excited for this feature. Every time I make a repo I regret not having it. Thanks @thenewpotato for your PR. |
@thenewpotato Just checking in on this. We just discussed this amongst the team and the approach outlined by @sergiou87 in #14943 (comment) is our preferred path forward. If you are able to and want to pick this up again, please let us know otherwise we will close this to open it up for other community contributions. |
@tidy-dev Thanks for bumping this thread. And thank you @nycdotnet for your comment! I like the approach @sergiou87 outlined and will be working on implementing it. |
Sorry about that @thenewpotato ! You're totally right. I opened a PR that refactors the I think it's nice to use the same component because this popover, unlike the generic In order to use it with a private renderPopoverDropdownButton = (props: PopoverDropdownAnchorProps) => {
return (
<LinkButton
onClick={props.onClick}
onAnchorRef={props.onAnchorRef}
>select a template</LinkButton>
)
} I still haven't discussed it with my teammates but I hope they find the changes in that PR acceptable. I will ping you as soon as that PR is merged (or if we decide to do something else). Thank you for your patience and sorry for the trouble 😓 |
Hi again @thenewpotato ! I'm afraid my PR won't be of much help here. In case you haven't noticed, lately we're doing a big effort to improve accessibility of GitHub Desktop, so we reached out our accessibility experts and they told us using a link for something other than navigating to a different part of the app is not a valid approach. Therefore we have to rethink the UI a bit to fit this "select template" action as a button that displays the popup. Just to give you a sneak peek of some ideas we're playing with, @tidy-dev suggested that we remove the hint text and just leave a button that triggers the popover: Please, wait a bit until we take a final decision. We will discuss about this internally and let you know about the path forward as soon as possible. Sorry for all this trouble. After all this time the team priorities changed to some extent and we have now more things to keep in mind for new features 😳 |
👋 @thenewpotato Sorry for such a long time circling back. We discussed this and have landed on: Using the new dropdown popover button with the words. "Replace with template" as shown in @sergiou87 above comment. Upon selection it will change to "Template: {selectedTemplate}". As for some discussed concerns:
Please let us know if you would like to continue working on this after such as long time, otherwise we will close in a few weeks. Sorry again for such delay. |
Closes #2197
Description
Allows users to use gitignore templates in existing repositories
.gitignore
is empty. The dropdown shows a list of available gitignore templates (same as the list of templates shown in the repository creation menu) that the user can select from.TextArea
with the content of the selected template. Users can make additional changes before saving the change.Screenshots
Release notes
Notes: