-
Notifications
You must be signed in to change notification settings - Fork 21
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
ENH - Improve default build selection workflow #271
Conversation
With the latest commit, the save.mp4 |
I feel like the text "Update Environment Build" isn't clear to end users. I think "Make Active" is clearer since we use the word "Active" in the dropdown. Along with that instead of called them "Builds" I feel "Versions" make more sense to an end user. |
Another thought. I don't think "Make Active" button belongs in the edit mode. We are not editing an environment just setting which one is the Active environment. I think it should show up in the default non-edit view but potentially only if you have permissions to make the change. |
I see; I think
Already raised some of this in #269 (comment) - the word
Smera suggested keeping this in the |
@trallard I'd be fine with us changing the terms we use in conda-store in the UI. I do think that environment version makes more sense.
I can't really make my mind on this one. I think it would be confusing in a "read only" view to expose something that could change the environment's state. |
I do not think it should be in read only - Smera already made really good points on the original issue about why this is best suited in Edit and I agree with this being a better location. But in general grouping workflows/concepts (edit, changes to settings, trigger builds) helps with user workflow streamlining and reduces context switching. |
I have tested the feature and it works. I can test again after the UI changes in discussion above are done, if that's the case. |
@steff456 - there are a number of changes/improvements needed:
|
I'm thinking about a new Nebari user who has no knowledge of conda-store. If they approach this button, will they know what "active" environment means? For example, maybe it just means active on the screen so they can edit it? What if we added a little question mark circle that leads to a modal explaining how "active" environments work in conda store? That would include details like
|
@kcpevey I think that should be part of the more extended conversation regarding the workflow. Plenty of improvements are needed in terms of notification/confirmation, documentation, etc., so let's keep this PR focused on splitting the workflows and taking the rest into a separate conversation. |
The last commit implemented all the changes needed for merging this PR. Now when there's a change in the specification, the dis.mp4Maybe after this PR we may need to revisit the disappearing of the |
This is another part of the workflow review we need to do, I would generally prefer that this is disabled/enabled rather than appearing/disappearing |
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.
Thanks @steff456 this looks almost ready.
I made a suggestion to rename variables to best reflect what they are for/when they are used. But I ran out of time to do all the search and replace.
Can you please do a thorough check/search/replace on your end so we can merge?
src/features/environmentDetails/components/EnvironmentDetails.tsx
Outdated
Show resolved
Hide resolved
src/features/environmentDetails/components/EnvironmentDetails.tsx
Outdated
Show resolved
Hide resolved
src/features/environmentDetails/components/EnvironmentDetails.tsx
Outdated
Show resolved
Hide resolved
src/features/environmentDetails/components/EnvironmentDetails.tsx
Outdated
Show resolved
Hide resolved
src/features/environmentDetails/components/Specification/SpecificationEdit.tsx
Outdated
Show resolved
Hide resolved
src/features/environmentDetails/components/Specification/SpecificationEdit.tsx
Show resolved
Hide resolved
src/features/requestedPackages/components/RequestedPackagesEdit.tsx
Outdated
Show resolved
Hide resolved
src/features/requestedPackages/components/RequestedPackagesTableRow.tsx
Outdated
Show resolved
Hide resolved
I just updated the variable names and double check that everything is working as expected. |
Part of #269.
Description
This pull request:
Before
Now
Pull request checklist
Additional information
Please note that the screenshots were taken with different themes