-
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
Alignment + zebra stripes for requested and installed packages #393
Conversation
Based on today's call, I will create a second PR to follow up this one in which I will add the "version installed" column to the top table (Requested Packages). Also:
|
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.
Are the changes to the ChannelsList
intended here? Seems like some styling stuff. As long as you're aware and sure about this, I'm okay with it.
About DependenciesItem
: I don't know if this element ever is used on its own, it seems like it's always wrapped in a TableRow
. Arguably you could just make the DependencyItem
a table row, moving one level inside the component. Really it seems fine as is though.
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.
I added some style tweaks.
width: isEditMode ? "140px" : "auto", | ||
marginLeft: isEditMode ? "20px" : "0px", |
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.
These alignment properties are no longer needed because the table layout provides the needed alignments.
I made the styling changes to the
Sure. I can make that change. Neither implementation (nether row nor cell) is ideal. But I don't want to refactor everything. |
Update: @smeragoel asked me to sync with her in person about this tomorrow or Thursday, so currently this PR is waiting on input from design. |
- apply monospace font to version numbers - Version Requested -> Requested Version
f058363
to
5458069
Compare
Sorry for the delay in the review! A minor nitpick would be if it is possible to use monospace font in the input text fields as well? Other than that, given that this is an "intermediate" fix and we already discussed that there might be more changes based on the results of the usability study, I think it is good to go. |
@smeragoel, for the input text fields, you only mean for the version and not the package name or constraint fields, is that correct? |
@smeragoel I updated the font in the form fields and updated the screenshots, could you take a quick look when you get a chance? |
@smeragoel I also moved the columns around in the edit view. It used to be: name, installed version, version constraint. For consistency, I changed it to: name, version constraint, installed version. This also made it easier to right-align the "installed version" column. |
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.
self review comments
@@ -1,7 +1,6 @@ | |||
export * from "./StyledAccordionSummary"; | |||
export * from "./StyledAccordionDetails"; | |||
export * from "./StyledAccordionTitle"; | |||
export * from "./StyledEditPackagesTableCell"; |
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.
Less code, hurray!
LGTM! Thanks for your work on this @gabalafou! |
Thanks @smeragoel! @peytondmurray if you could give this one last look (perhaps using the "changes since your last review" view), that would be great! |
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.
I didn't look closely at styles since @smeragoel did, but the rest of this is great. Loving the level of clarity this provides to the user, AFAIK no other package/environment management system does this. 🚀
page.locator( | ||
"#infScroll > .infinite-scroll-component__outerdiv > .infinite-scroll-component > div > div > .MuiButtonBase-root" | ||
).first.click() | ||
page.get_by_test_id("PromoteIcon").first.click() |
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.
Much cleaner! 🚀
Thanks, Peyton! |
Reference: #268.
Description
This PR partially implements the designs set out in 268 (comment, Figma).
For a first pass, I opted for an implementation that doesn't require refactoring the frontend code.
The design in #268 adds a "Version installed" column to the top table ("Requested packages"). That would require some refactoring, so instead I decided to keep the two tables semantically distinct. In this PR, the top table shows the list of packages that were requested, while the bottom table shows all of the packages that were installed (this includes the requested packages and all of their dependencies). Screenshots below.
I also ignored a few stylistic details in the design. For example, there are no horizontal lines in the table designs but there are in this PR. This is because MUI's tables include the lines automatically, and we decided in a previous team meeting that we would stick to MUI default styles in the interest of time.
I believe that this PR improves the existing UI, so even though it doesn't implement all of the design changes in #268, we should be able to merge this PR and then create another PR later to implement the other changes if we decide that's what we still want to do.
Pull request checklist
Screenshots
Create new environment
View environment
Edit environment