feat: add warning when navigating away with unsaved changes in environment settings #5049
Conversation
…ctor/migrate-environment-settings-page-to-ts
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
@Zaimwa9 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
the
front-end
| @@ -79,14 +79,14 @@ const Tabs = class extends React.Component { | |||
| } | |||
| this.props.onChange?.(i) | |||
| }} | |||
| className={`btn-tab ${isSelected ? ' tab-active' : ''}`} | |||
| className={`btn-tab ${this.props.noFocus ? 'btn-no-focus' : ''} ${isSelected ? ' tab-active' : ''}`} | |||
There was a problem hiding this comment.
This is a very specific implementation related to the hook. The selected tab would keep the focus on after interacting with the newly implemented modal
Uffizzi Ephemeral Environment
|
Uffizzi Ephemeral Environment Deploying☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/5049 ⚙️ Updating now by workflow run 13027216418. What is Uffizzi? Learn more! |
| {currentEnv?.use_v2_feature_versioning === false && ( | ||
| <EnvironmentVersioningListener | ||
| // TODO: Should be listening on Id or API_KEY ? | ||
| id={env.api_key} |
There was a problem hiding this comment.
Not sure here what should be the key
There was a problem hiding this comment.
It's the api key, this component polls the environment when enabling feature versioning (an upcoming feature).
The process is async, saving the environment doesn't process right away so we listen for a change in data and respond to it.
| <ProjectProvider | ||
| onRemoveEnvironment={onRemoveEnvironment} | ||
| id={match.params.projectId} | ||
| onRemove={onRemove} | ||
| onSave={onSave} | ||
| > | ||
| {({ deleteEnv, isLoading, isSaving, project }) => { | ||
| const env = _.find(project?.environments, { | ||
| api_key: match.params.environmentId, | ||
| }) |
There was a problem hiding this comment.
Based on the other usage of ProjectProvider, I would suggest to use it:
- Either as a context if you need to have the projects method available through the whole app
- As a custom hook so the methods can be accessed only in necessary component
I do get that this is the most straightforward to live with Functional/Class components but as of, it enforces to provide a function as a children.
The file is quite massive (~900 lines) and could benefit from being broken in pieces but it would be way easier by dealing only with FC (especially as deleteEnv is used only by one sub-component.
One or the other method would allow to use the methods in a more fine-grained way while avoid props drilling.
From my understanding, it wouldn't impact the event listeners wrapped within Project Provider
There was a problem hiding this comment.
This (as well as corresponding app-actions/action-constants) will eventually be removed when project-store.js is migrated to RTK code our CLI generates. ProjectProvider will simply be replaced by references to generated hooks inside of useEnvironment.ts/useProject.ts
| return ( | ||
| <> | ||
| <DirtyFormModal /> | ||
| <PageTitle title='Settings' /> |
There was a problem hiding this comment.
Overall, I believe the page could be broken up in a lot of pieces and sub-form that would be in charge of partially update necessary part of the environment:
- Each tab could be its own component.
- General settings could be an isolated form (especially as it is the only one being submitted)
- Same for the switch part etc
There was a problem hiding this comment.
Yep agree that separating the form and other components makes sense.
| }) | ||
| } | ||
|
|
||
| const saveEnv = (newEnv: Partial<Environment> = {}) => { |
There was a problem hiding this comment.
So here I modified it in a way that it is not reliant on the form event (which was not used).
Basically, it will update the currentEnv state with the new specific fields/value provided.
I think there is some room to improve robustness by splitting up the component and have smaller/more specific forms with precise typing 👍
There was a problem hiding this comment.
Yeah, moving to a functional component / using useUpdateEnvironmentMutation will totally remove all of the nasty state mutation stuff it's doing.
| use_mv_v2_evaluation: !!editedEnv?.use_mv_v2_evaluation, | ||
| }), | ||
| ) | ||
| setIsDirty(false) |
There was a problem hiding this comment.
On saving, we reset isDirty
| } | ||
|
|
||
| const updateCurrentEnv = (newEnv: Partial<Environment> = {}, shouldSaveUpdate?: boolean, isDirtyDisabled?: boolean) => { | ||
| if (!isDirtyDisabled) { |
There was a problem hiding this comment.
This is a flag to avoid "dirtying" the form. It was necessary for the feature versioning toggle (conflict with 2 different keys enabledFeatureVersioning, use_v2_feature_versioning).
if you have some more context on these 2 keys I can give it another look :)
There was a problem hiding this comment.
Then isDirty is set to true, on saving it's resetted
|
|
||
| EnvironmentSettingsPageTS.displayName = 'EnvironmentSettingsPage' | ||
|
|
||
| export default ConfigProvider(withWebhooks(EnvironmentSettingsPageTS)) |
There was a problem hiding this comment.
Same here, I do think withWebhooks could be turned into a hook so there is no need to pass the props from above.
The methods are not dynamic so I believe it's a good use case to turn it into a hook and avoid an additional wrapping of child component
There was a problem hiding this comment.
Yeah actually this is probably the easiest migration to RTK I can think of @tiagoapolo
| webhooks: Webhook[] | ||
| webhooksLoading: boolean | ||
| getWebhooks: () => void | ||
| createWebhook: (webhook: Partial<Webhook>) => Promise<void> | ||
| saveWebhook: (webhook: Webhook) => Promise<void> | ||
| deleteWebhook: (webhook: Webhook) => Promise<void> |
There was a problem hiding this comment.
CF. comment below, using a hook would avoid to pass them as props
|
I added a couple of comments! Waiting for your review, don't hesitate to share feedback/questions and or requests for changes. On a side note, I do think it could be interesting to add tailwind as from what I saw you have implemented some classes that would work exactly the same ( And about my comments, I just wanted to have it written somewhere if ever they get useful the day you have the bandwidth (as they are mostly code-maintainability related) 👍 |
| <Button onClick={discardAndConfirmNavigation} theme="secondary" className="mr-2">Yes, discard changes</Button> | ||
| <Button onClick={cancelNavigation} class="btn-primary">Cancel</Button> |
There was a problem hiding this comment.
Let me know if I need to invert the colors/theming!
There was a problem hiding this comment.
Awesome! I guess we can start using this in a lot of places
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature!Changes
#4922
Migrated EnvironmentSettingsPage to Typescript
common/types/responsescommon/types/webhooksCreated useFormNotSavedModal
isDirtyandisNavigating(to allow custom modal on SPA navigation) stateI have a couple of feedback / future improvements that I will put in the code of the PR directly :)
How did you test this code?
Disclaimer: I haven't tested yet the enterprise features, if you have any tips to test it easily I will add them. I will also do it in the coming days
Custom not saved modal
https://www.loom.com/share/23d7de64ac2b43c496e249880ae4174f
Browser default popup on refresh
Testing ISO behavior
https://www.loom.com/share/8dacdcac0d8649abacea9d0275bb3392