-
Notifications
You must be signed in to change notification settings - Fork 605
fix: ensure template toggle overrides blank content (#7556) only when… #7748
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
base: master
Are you sure you want to change the base?
fix: ensure template toggle overrides blank content (#7556) only when… #7748
Conversation
… only when body is empty
@TomasGonzalez is attempting to deploy a commit to the GitButler Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hola!
Thanks so much for your contribution. It's really appreciated :)
I left some feedback here for which I would be very interesting in hearing your thoughts about.
The changes don't seem to solve the specified problem as described in the issue #7556. Deleting the content in the PR modal doesn't display the template again. Look in the prContents.svelte.ts file. The fix should probably go in there :) |
Hola! thanks for the feedback! From my interpretation of the issue (#7556), the problem described was that after clearing the body content, the user couldn't re-activate the template again via the UI (i.e., toggling the template switch off and back on didn't restore the template). This happened because the previously saved (blank) body was persisted in local storage and prevented reloading the template. My fix addresses this by ensuring that if the body content is cleared, the persistence state is also cleared—allowing the template to be toggled back on explicitly by the user afterward. Automatically re-inserting the template immediately after clearing the body content might not be ideal, as clearing could be intentional. Additionally, it would cause unexpected behavior: if the user deliberately deletes the template text, it would immediately reappear. My approach ensures the user explicitly decides when to restore the template, aligning more intuitively with typical user expectations. That said, if the intended UX is to instantly re-display the template upon clearing the body, we could discuss adjustments in the prContents.svelte.ts component. Let me know your thoughts! |
Hey Tomas, thanks for addressing the feedback!
I see. Yes, that works as specified 👍
This is a good point. I guess is about picking which one is the least unexpected behavior:
My gut feeling is that the former is the preferable one. What do you think? If you agree, could you adjust it so that it behaves like that instead? |
Thanks for the follow-up! I totally get where you're coming from, this does feel like a case of choosing the least unexpected behavior for users. Thinking about it a bit more though, I wonder if the real issue might be the UI model itself. Having a toggle control whether the template is applied feels a bit limited, especially since users can already select a specific template manually. Maybe a cleaner approach would be to let users re-apply the selected template from the list at any time, rather than relying on the toggle to "force" it. That way the action is explicit, there’s no risk of overwriting on accident. Totally open to going with your suggestion if that’s the direction you prefer, just wanted to throw this in as an alternative in case it resonates :) |
It may be that. How I think of the current way the UI presents this option, is that the template is just the persisted starting point of whatever body content you want input/generate.
That said, maybe a completely different UI layout might be less cumbersome. If you're still willing to indulge this further I propose the following: What do you think? |
Here’s an updated design in Figma for the "Submit for Review" modal. It’s now a unified flow for both "Create a PR" and "Create a BR," rather than just a PR creation.
![]() |
@PavelLaptev Nice!
I think we shouldn't preselect a template. Instead, we can have an initial option I think it shouldn't say |
Hey @PavelLaptev, this updated design looks awesome and super clean, I’m definitely in favor of dropping the toggle. Totally agree that “No template” makes sense as the default, and no need for a “Please select…” prompt. One small thing I’d love to confirm: would it be possible for the dropdown to allow re-selecting the same template to overwrite the body? That way, if I clear the text and want to bring the template back, I can just select it again. I prefer that to it auto-repopulating when the field is empty but happy to go with whatever fits best! Let me know what you think and I’ll get started on the implementation :) |
Hey @estib-vega Just a quick update, I went ahead and implemented the behavior as originally discussed: clearing the body re-applies the template if the toggle is still on. I’d love to explore the updated design further, but realistically I don’t have the bandwidth to take that on unless I were more deeply involved with the project. That said, happy to offer thoughts or feedback if it moves forward later on :) |
@TomasGonzalez Thanks! We had a quick discussion about the "Submit for Review" modal design. Just small UI updates. I'll add a link here once I’ve updated it in Figma. |
@PavelLaptev Great, feel free to ping me when it’s ready! |
Hey 😊 The current "Submit for Review" design is here in Figma. |
Hello GitButler team! 👋
I'm Tomás, a software engineer who recently applied for the
Senior TypeScript Developer
position in Berlin. While familiarizing myself with your product, I noticed this issue (#7556 ) and wanted to contribute a small improvement. I'm genuinely excited about GitButler's mission and would love to join and contribute more in the future. 🚀🧢 Changes
☕️ Reasoning
The main fix ensures that when the template toggle is turned on, it properly overrides blank content as expected.
While working on this, I refactored the method for selecting a template for clarity while investigating why the templates I had in my root directory were not being read. I ensured the logic remains unchanged but easier to follow.
Additionally, I realized that the Rust helper only checks inside the directory specified for the current forge (in my case, .github) for templates, whereas GitHub itself recognizes template files located in the repository root as well. This might not be directly related to the issue at hand but could be worth investigating separately.