Add option to disable commit message length warning #17370
Conversation
There was a problem hiding this comment.
@rystills Thank you for this PR! We actually had this work planned soon as we are intending to refactor the commit length warning to be a little more apparent for accessibility purposes.
This appear to be on the right track. We have a few requests that you can find in a various PR review comments, but to recap:
- We would prefer this to live under the "Prompts" settings.
- The state and local storage to be handled in the
app-stateandapp-store.
There is one last thing to consider and I am going to check with my teammates on it and then circle back. That is, we would like this to be false by default for new installations, but keep it enabled for current installations. We may do it based on version number, but have not found an example of this. Alternatively, we ship it as true by default and then flip it later down the road when we add in the accessibility work.
app/src/lib/stores/app-store.ts
Outdated
| @@ -554,6 +556,8 @@ export class AppStore extends TypedBaseStore<IAppState> { | |||
|
|
|||
| if (__WIN32__) { | |||
| const useWindowsOpenSSH = getBoolean(UseWindowsOpenSSHKey) | |||
| // TODO: is it ok to use a string literal here? | |||
There was a problem hiding this comment.
Looking at the implementation of askForConfirmationOnRepositoryRemoval may be helpful reference as it is a similar setting.
Preferably,
- we set a key as constant at the top of file, see the
confirmRepoRemovalKeyas the example for theaskForConfirmationOnRepositoryRemovalprompt setting. - Additionally, as the location of this initialization is not quite ideal especially given this is in a Windows only if block and we would like applied to all operating systems. We typically initialize these in the
loadInitialState()method. You can seeaskForConfirmationOnRepositoryRemovalon line 2095. - Also add a 'Default" const, you can see this as
confirmRepoRemovalDefaultin the above example. Set it to true for now please.
app/src/lib/stores/app-store.ts
Outdated
| @@ -3532,6 +3537,12 @@ export class AppStore extends TypedBaseStore<IAppState> { | |||
| this.emitUpdate() | |||
| } | |||
|
|
|||
| public _setShowCommitLengthWarning(showCommitLengthWarning: boolean) { | |||
| setBoolean("showCommitLengthWarning", showCommitLengthWarning) | |||
There was a problem hiding this comment.
Just noting again for other comment -> prefer the use of a constant set at the top of file.
| @@ -235,6 +237,9 @@ export class CommitMessage extends React.Component< | |||
| isCommittingStatusMessage: '', | |||
| repoRulesEnabled: false, | |||
| isRuleFailurePopoverOpen: false, | |||
| // TODO: do a proper retrieval rather than pinging localStorage (use AppState?) | |||
| // (current solution requires a tab switch to hot reload) | |||
| showCommitLengthWarning: localStorage.getItem("showCommitLengthWarning") === "1", | |||
There was a problem hiding this comment.
The getBoolean you are using on line 556 is the call to the localStorage so you were already on the right track. :) When you put that in the loadInitialState, it will populate the appState correctly from local storage. Then, you need to pass it down through the props, I believe if you follow commitSpellcheckEnabled, you can see how to it gets from App.tsx in the renderRepository down through multiple components to this file. :)
Then you will no longer needs a showCommitLengthWarning state variable on this component as you will be able to use this.props.showCommitLengthWarning
Hi @tidy-dev. I don't work with typescript very often so I appreciate the thorough feedback! I made the requested changes but I ended up adjusting more files than I expected, so please let me know if any of my changes are redundant and should be removed. Regarding the default value, I can't think of anything clever beyond checking the version number on init like you mentioned, but I'll be sure to keep it in mind. |
There was a problem hiding this comment.
@rystills Thank you for the changes and for the wait on further feedback. I went investigating what we might like to do for local storage management and just got the chance to circle back.
Couple things:
- I have added many suggestions to remove the prop passing that appears unused. I am assuming you likely added them since I gave the
commitSpellCheckexample. ThecommitSpellCheckhad a bit more use case then this tho. We only want to add this where is is currently being used. - local storage management -> I am exploring another possible route with our local storage management and thus for now, I have a suggestion to copy an existing pattern in the code for storing a current behavior. We will handle switching that default in a later PR. :)
Co-authored-by: tidy-dev <75402236+tidy-dev@users.noreply.github.com>
Thanks for the assistance. I figured most of that property passing was unnecessary, but I wasn't familiar enough with the dependency graph to confidently prune it. Should be all set now. |
Partially satisfies #14255
Description
This PR adds a checkbox to the advanced settings to toggle the 50-character commit message length nag.
2 TODOs were added due to my uncertainty in accessing properties and in localStorage string literal storage (these should be resolved before merging).
Screenshots
Release notes
Notes: added commit message length warning toggle