Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: update Column and Row from Hew #8412

Merged
merged 12 commits into from
Nov 15, 2023
Merged

chore: update Column and Row from Hew #8412

merged 12 commits into from
Nov 15, 2023

Conversation

johnkim-det
Copy link
Contributor

Description

Updating changes from Hew

Test Plan

No feature testing required

Commentary (optional)

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

WEB-1722

@johnkim-det johnkim-det requested a review from a team as a code owner November 14, 2023 19:42
@cla-bot cla-bot bot added the cla-signed label Nov 14, 2023
@netlify Netlify
Copy link

netlify bot commented Nov 14, 2023 •
edited
Loading

✅ Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit f8212db
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/6554ed1a4218270008af233d
😎 Deploy Preview https://deploy-preview-8412--determined-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

EmilyBonar
EmilyBonar requested changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@EmilyBonar EmilyBonar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why I didn't catch this in the hew PR, but it seems like alignment isn't working correctly.

webui/react/src/pages/F_ExpList/glide-table/TableActionBar.tsx Outdated Show resolved Hide resolved
webui/react/src/pages/WorkspaceDetails/WorkspaceProjects.tsx
</Column>
</Columns>
</Column>
<Column align="right">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: again, this alignment isn't working
Screenshot 2023-11-14 at 15-32-12 Determined

webui/react/src/pages/WorkspaceList.tsx
</Column>
</Columns>
</Column>
<Column align="right">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: alignment not working

webui/react/src/pages/WorkspaceList/WorkspaceCard.tsx Outdated Show resolved Hide resolved
webui/react/src/pages/F_ExpList/F_ExperimentList.tsx
@@ -817,7 +818,7 @@ const F_ExperimentList: React.FC<Props> = ({ project }) => {
/>
</ComparisonView>
{showPagination && (
<Columns>
<Row>
<Column align="right">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: alignment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These buttons should be aligned to the right side of the screen
Screenshot 2023-11-14 at 16-31-21 Uncategorized Experiments - Determined

webui/react/src/pages/Admin/UserManagement.tsx Show resolved Hide resolved
@johnkim-det
Copy link
Contributor Author

thanks for catching these. I think the Column width prop addresses what you were seeing. I actually was not able to reproduce on F_ExperimentList, WorkspaceList, and WorkspaceProjects, but added it there too anyway.

EmilyBonar
EmilyBonar requested changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@EmilyBonar EmilyBonar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several still aren't working

webui/react/src/pages/F_ExpList/glide-table/TableActionBar.tsx Show resolved Hide resolved
webui/react/src/pages/F_ExpList/F_ExperimentList.tsx Outdated Show resolved Hide resolved
webui/react/src/pages/Admin/UserManagement.tsx Outdated
</Column>
<Column align="right">
<Columns>
<Column align="right" width="hug">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: i don't understand how making the container hug the contents is supposed to move the "Actions" and "Add User" buttons to the right
Screenshot 2023-11-14 at 16-33-08 Determined

webui/react/src/pages/F_ExpList/F_ExperimentList.tsx
@@ -817,7 +818,7 @@ const F_ExperimentList: React.FC<Props> = ({ project }) => {
/>
</ComparisonView>
{showPagination && (
<Columns>
<Row>
<Column align="right">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These buttons should be aligned to the right side of the screen
Screenshot 2023-11-14 at 16-31-21 Uncategorized Experiments - Determined

@johnkim-det
Copy link
Contributor Author

looks like the problem was related to Row always having 100% width, which causes alignment issues when a Row is a child of a Column. addressing in determined-ai/hew#44.

@johnkim-det johnkim-det mentioned this pull request Nov 15, 2023
Closed
4 tasks
EmilyBonar
EmilyBonar approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@EmilyBonar EmilyBonar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@johnkim-det johnkim-det merged commit d4c4195 into main Nov 15, 2023
69 of 80 checks passed
@johnkim-det johnkim-det deleted the WEB-1722 branch November 15, 2023 16:17
@NicholasBlaskey NicholasBlaskey added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Nov 15, 2023
dai-release bot pushed a commit that referenced this pull request Nov 15, 2023
@johnkim-det @determined-ci
chore: update Column and Row from Hew (#8412)
b8c1be7 
(cherry picked from commit d4c4195)
@dannysauer dannysauer added this to the 0.26.5 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ClaireNeveu ClaireNeveu ClaireNeveu left review comments

@EmilyBonar EmilyBonar EmilyBonar approved these changes

@ashtonG ashtonG Awaiting requested review from ashtonG ashtonG is a code owner automatically assigned from determined-ai/web

Assignees

@EmilyBonar EmilyBonar

Labels
cla-signed to-cherry-pick Pull requests that need to be cherry-picked into the current release
Projects
None yet
Milestone
0.26.5
Development

Successfully merging this pull request may close these issues.
5 participants
@johnkim-det @ClaireNeveu @EmilyBonar @dannysauer @NicholasBlaskey