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.

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

fix: ensure all columns have widths #9136

Merged
merged 2 commits into from
Apr 11, 2024
Merged

fix: ensure all columns have widths #9136

merged 2 commits into from
Apr 11, 2024

Conversation

ashtonG
Copy link
Contributor

@ashtonG ashtonG commented Apr 9, 2024

Ticket

ET-161

Description

this is an alternate fix for a bug where updating the compare width would cause the experiment list settings to be in an invalid state. It seems like the issue was due to settings widths not containing pinned columns, so we try to update the column widths along with the columns when they update.

Test Plan

  • Reset user settings
  • visit an experiment list page
  • open compare mode
  • click and drag the divider
  • the pinned columns in the left panel should shrink and grow with the divider position

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.

this is an alternate fix for a bug where updating the compare width
would cause the experiment list settings to be in an invalid state. It
seems like the issue was due to settings widths not containing pinned
columns, so we try to update the column widths along with the columns
when they update.
@ashtonG ashtonG requested a review from a team as a code owner April 9, 2024 20:19
@ashtonG ashtonG requested a review from gt2345 April 9, 2024 20:19
@cla-bot cla-bot bot added the cla-signed label Apr 9, 2024
Copy link

codecov bot commented Apr 9, 2024

Codecov Report

Attention: Patch coverage is 4.76190% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 39.52%. Comparing base (133f838) to head (48c8b40).
Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9136      +/-   ##
==========================================
- Coverage   46.16%   39.52%   -6.64%     
==========================================
  Files        1174      852     -322     
  Lines      145187   105835   -39352     
  Branches     2413     2413              
==========================================
- Hits        67025    41832   -25193     
+ Misses      77954    63795   -14159     
  Partials      208      208              
Flag Coverage Δ
harness ?
web 36.81% <4.76%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...t/src/pages/F_ExpList/F_ExperimentList.settings.ts 100.00% <100.00%> (ø)
...bui/react/src/pages/F_ExpList/F_ExperimentList.tsx 0.00% <0.00%> (ø)

... and 322 files with indirect coverage changes

Copy link

netlify bot commented Apr 9, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 48c8b40
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/6616fc278fa24f0008d298fc
😎 Deploy Preview https://deploy-preview-9136--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.

@ashtonG ashtonG requested review from EmilyBonar and removed request for gt2345 April 9, 2024 21:11
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.

Playing around with it, I'm seeing that sometimes the sum of the pinned column widths doesn't match the width of the table as a whole in the compare view.

Screenshot 2024-04-10 at 15-44-03 Uncategorized Experiments - Determined

@ashtonG
Copy link
Contributor Author

ashtonG commented Apr 10, 2024

@EmilyBonar we avoid adjusting the column widths below the minimum width when adjusting the comparison width, but it seems we don't do that when adjusting the column size itself -- i just pushed a fix for that -- can you verify?

@EmilyBonar
Copy link
Contributor

@EmilyBonar we avoid adjusting the column widths below the minimum width when adjusting the comparison width, but it seems we don't do that when adjusting the column size itself -- i just pushed a fix for that -- can you verify?

Yes, that seems to be working. I'm still seeing the shadow shown in the previous screenshot though.

@ashtonG
Copy link
Contributor Author

ashtonG commented Apr 10, 2024

i think the shadow might be tied to the split pane component and how it throttles the size changes. Can we make that its own issue please?

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.

LGTM

@ashtonG ashtonG added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Apr 11, 2024
@ashtonG ashtonG merged commit 95f87d7 into main Apr 11, 2024
74 of 86 checks passed
@ashtonG ashtonG deleted the fix/prevent-bad-settings branch April 11, 2024 16:03
dai-release bot pushed a commit that referenced this pull request Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed to-cherry-pick Pull requests that need to be cherry-picked into the current release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants