feat: show and hide environments #9323
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
There was a problem hiding this comment.
Copy for keeping a new version behind a flag
| environments: Pick<FeatureEnvironmentSchema, 'name'>[]; | ||
| hiddenEnvironments: FeatureEnvironmentSchema['name'][]; | ||
| onChange: (name: FeatureEnvironmentSchema['name']) => void; |
There was a problem hiding this comment.
These are all strings or simple objects, right? Is there a particular benefit to depending on the openapi types for them? I don't see how they'll ever change, and these types stand out a lot more than just string or {name: string}.
Just thinking that if you're not that familiar with the schemas, you might need to go and look them up and everything, and I don't see what it adds here aside from ... more text.
I don't mind particularly, but I'd maybe stick with simpler types here instead. Especially for when the type is just string. I think there's a better case for using the Pick type.
| environments: Pick<FeatureEnvironmentSchema, 'name'>[]; | |
| hiddenEnvironments: FeatureEnvironmentSchema['name'][]; | |
| onChange: (name: FeatureEnvironmentSchema['name']) => void; | |
| environments: {name: string}[]; | |
| hiddenEnvironments: string[]; | |
| onChange: (name: string) => void; |
But anyway: up to you 🤷🏼
...tureOverview/FeatureOverviewMetaData/EnvironmentVisibilityMenu/EnvironmentVisibilityMenu.tsx
Show resolved
Hide resolved
| <Checkbox | ||
| onChange={() => onChange(name)} | ||
| checked={!hiddenEnvironments?.includes(name)} | ||
| /> |
There was a problem hiding this comment.
Again: I'm not sure how Menu works, but there's a couple things here that we might wanna change:
All form controls (that includes checkboxes), should have labels (according to MUI's accessibility notes for this component). In this case, you'd probably want to label it with the name of the env (so probably wrap the name of the env in a label and mark it as being for this checkbox).
But I'm not even sure we should have the checkbox here. How does it work with keyboard focus? Does it give you multiple tab stops within it? It looks like it's designed to be navigated by arrow keys and that tab takes you to the next item in the document flow, but I'm not sure whether adding an input field here messes with that or not. Regardless, I can imagine it being confusing for screen readers.
But based on how MUI uses the menu in their docs, maybe it's better to just use an icon instead of a form control? 💁🏼
...Overview/FeatureOverviewMetaData/EnvironmentVisibilityMenu/hooks/useEnvironmentVisibility.ts
Outdated
Show resolved
Hide resolved
| {onEnvironmentVisibilityChange ? ( | ||
| <EnvironmentVisibilityMenu | ||
| environments={feature.environments || []} | ||
| hiddenEnvironments={hiddenEnvironments || []} | ||
| onChange={onEnvironmentVisibilityChange} | ||
| /> | ||
| ) : null} |
There was a problem hiding this comment.
Why is the onEnvironmentVisibilityChange function optional? Are there cases where we don't want you to do that?
There was a problem hiding this comment.
For tests that. We have some "parent flag" tests that don't pass environments.
There was a problem hiding this comment.
Hmm. I'm not sure I want the tests to determine whether something should be required or not. And in this case, seems like we fall back to an empty list of environments anyway? Optionally, you could keep the function optional but give it a default impl that does nothing?
...FeatureView/FeatureOverview/NewFeatureOverviewEnvironment/FeatureOverviewEnvironmentBody.tsx
Outdated
Show resolved
Hide resolved
.../FeatureView/FeatureOverview/NewFeatureOverviewEnvironment/NewFeatureOverviewEnvironment.tsx
Show resolved
Hide resolved
.../FeatureView/FeatureOverview/NewFeatureOverviewEnvironment/NewFeatureOverviewEnvironment.tsx
Show resolved
Hide resolved
| /** | ||
| * @deprecated use tested `useLocalStorageState` hook instead | ||
| */ |
There was a problem hiding this comment.
How many places do we use this today? I migrating off it going to be hard? Also, why are we deprecating it? I don't remember any conversation about that.
There was a problem hiding this comment.
Just this place. I didn't find it anywhere else. We already got rid of this for favorites
There was a problem hiding this comment.
Safe. Shouldn't be a problem, then. But I still don't understand why we're deprecating it? Does it do something we've moved away from? Does it not work?
…tureOverviewMetaData/EnvironmentVisibilityMenu/hooks/useEnvironmentVisibility.ts Co-authored-by: Thomas Heartman <[email protected]>
About the changes