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

chore: TrialsComparisonModal style fixes [WEB-1919] [WEB-1909] #8674

Merged
merged 6 commits into from
Jan 17, 2024

Conversation

mapmeld
Copy link
Contributor

@mapmeld mapmeld commented Jan 10, 2024

Description

Addresses an Antd / UI Kit ticket, plus a bug ticket, around the TrialsComparisonModal

  • Replaces antd.Tag with Row containing Label and close icon button
  • Updates colSpan to cover multiple experiments or multiple trialIds (+1 to include original column)

As used in multi-trial experiment trials table:

Screen Shot 2024-01-10 at 5 03 44 PM

As used in the experiments Glide Table:

Screen Shot 2024-01-10 at 5 03 32 PM

Test Plan

Open a multi-trial experiment such as /det/experiments/1290/trials. Check to select multiple trial rows. In the action menu, select Compare trials to open the modal

Open a project such as /det/projects/1/experiments. Check multiple successful / completed experiments which can be compared.
Open the Compare section using the button at top right of the table.
In the Compare section, click the Details tab to see the table as used here

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.

@cla-bot cla-bot bot added the cla-signed label Jan 10, 2024
Copy link

netlify bot commented Jan 10, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 1b9438d
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65a81a4939911300084c26da
😎 Deploy Preview https://deploy-preview-8674--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.

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (6d3f9ff) 51.51% compared to head (1b9438d) 47.32%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8674      +/-   ##
==========================================
- Coverage   51.51%   47.32%   -4.19%     
==========================================
  Files         887      570     -317     
  Lines      156209   117679   -38530     
  Branches     2088     2088              
==========================================
- Hits        80466    55693   -24773     
+ Misses      74250    60493   -13757     
  Partials     1493     1493              
Flag Coverage Δ
harness ?
web 53.77% <14.28%> (+<0.01%) ⬆️

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

Files Coverage Δ
.../pages/ExperimentDetails/TrialsComparisonModal.tsx 13.47% <14.28%> (+0.41%) ⬆️

... and 317 files with indirect coverage changes

@mapmeld mapmeld marked this pull request as ready for review January 11, 2024 15:52
@mapmeld mapmeld requested a review from a team as a code owner January 11, 2024 15:52
@mapmeld mapmeld requested review from keita-determined and removed request for johnkim-det January 16, 2024 16:36
Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

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

minor bug
the x button's z-index value looks incorrect

Screen.Recording.2024-01-16.at.8.55.51.AM.mov

@mapmeld
Copy link
Contributor Author

mapmeld commented Jan 16, 2024

@keita-determined good catch! I selected more experiments in Uncategorized so that the issue would be visible. Then I added a z-index css rule to the top-left table header (adding z-index to the button did not help)

@mapmeld mapmeld assigned keita-determined and unassigned mapmeld Jan 16, 2024
Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

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

one more little thing: header z-index should be bigger than dropdown

Screen.Recording.2024-01-16.at.11.10.25.AM.mov

also X button in the header doesnt do anything.

@mapmeld
Copy link
Contributor Author

mapmeld commented Jan 16, 2024

I think this new z-index gives us the right order of z-index es

The left column cannot be visible in the top left unless we made a special case for dropdowns

Screen Shot 2024-01-16 at 1 40 44 PM

@mapmeld mapmeld assigned keita-determined and unassigned mapmeld Jan 16, 2024
Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

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

LGTM

@mapmeld mapmeld merged commit 2e60167 into main Jan 17, 2024
69 of 81 checks passed
@mapmeld mapmeld deleted the compare_trials branch January 17, 2024 18:26
maxrussell pushed a commit that referenced this pull request Mar 21, 2024
* z-index in header
* hide column unselect if not in use; useMemo on right pane
* stop playwright at v1.40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants