fix: description state management in EnvironmentSettingsPage #4917
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
github-actions
bot
added
front-end
Docker builds report
|
tiagoapolo
requested review from
zachaysan,
matthewelwell,
a team and
kyle-ssg
and removed request for
kyle-ssg and
a team
|
Uffizzi Preview |
| @@ -61,7 +61,7 @@ const EnvironmentSettingsPage = class extends Component { | |||
| const env = ProjectStore.getEnvs().find( | |||
| (v) => v.api_key === this.props.match.params.environmentId, | |||
| ) | |||
| this.setState({ env }) | |||
| this.setState({ env, description: env.description ?? "" }) | |||
There was a problem hiding this comment.
I'm interested to know why description has to be handled differently? Shouldn't it just be updated as part of the environment object? 🤔
There was a problem hiding this comment.
My approach here was to make sure we're consistent in how we're getting and updating the data from, previously we were mixing internal states with context one, which in my opinion is more confusing and prone to bugs. So, either using environment object or description internal states is fine.
value={
typeof this.state.description === 'string'
? this.state.description
: env.description
}
Ref: #4841 check for empty roles list unifies description state management from env state check roles
tiagoapolo
force-pushed
the
fix/env-description
branch
from
7b00299 to
b83d894
Compare
There was a problem hiding this comment.
This looks good to me now.
The only think that I noticed which might be relevant to think about (in a separate issue) is the fact that there is no warning that any in progress edits will be lost when switching between environments.
So, for example, if you're editing the description of your development environment, then switch to your production environment to check something, your edits from the development environment will be lost. I guess the slightly interesting quirk of the previous (buggy) behaviour is that this wasn't an issue!
@tiagoapolo can you create a separate issue to tackle this? I think we should just add a warning when you navigate away if the environment settings haven't been saved.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature!Changes
Before
not_fixed.mov
After
fixed.mov
How did you test this code?
Please describe.