Skip to content

Match the way Studio displays experiment name information in the table#3723

Merged
mattseddon merged 3 commits intomainfrom
match-studio-display
Apr 21, 2023
Merged

Match the way Studio displays experiment name information in the table#3723
mattseddon merged 3 commits intomainfrom
match-studio-display

Conversation

@mattseddon
Copy link
Contributor

@mattseddon mattseddon commented Apr 20, 2023

First follow-up to this comment #3619 (comment).

For reference, this is how Studio displays an experiment's sha/name:

image

As you can see the experiment name has been emphasised as this is the human-readable part of the information displayed.

Demo

Screen.Recording.2023-04-20.at.11.56.35.am.mov

See the storybook for further examples.

I think that consolidating between the products is a good idea but you may disagree. Maybe we should actually be swapping the experiment name and short sha's positions.

LMK what you think.

@mattseddon mattseddon added the product PR that affects product label Apr 20, 2023
@mattseddon mattseddon self-assigned this Apr 20, 2023
Copy link

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 37618 lines exceeds the maximum allowed for the inline comments feature.

@mattseddon mattseddon changed the base branch from main to integrate-exp-show April 20, 2023 01:58
@mattseddon mattseddon force-pushed the match-studio-display branch from 8454a14 to e34f534 Compare April 20, 2023 02:01
@mattseddon mattseddon marked this pull request as ready for review April 20, 2023 02:02
@mattseddon mattseddon changed the title Match the way Studio displays experiment name information Match the way Studio displays experiment name information in the table Apr 20, 2023
Copy link
Contributor

@sroy3 sroy3 left a comment

Choose a reason for hiding this comment

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

Is it normal that the comparison table images changed in Storybook?

/>
) : (
<div className={styles.experimentCellText}>
<span
Copy link
Contributor

Choose a reason for hiding this comment

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

I was staring at the table for like ten minutes, trying to figure out why it looked so off to me when it finally hit me that the commit rows have secondary text second while the experiments have their secondary text first.

I'm assuming most people won't notice 😅, but maybe we should keep secondary text second across all rows? Example of how this would look:

image

Technically, in design, you can have secondary text first depending on other factors but the main reason that the rows were bothering me is the fact we have a mixture of secondary being first and second. Though again, I'm probably the only person getting bothered :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that this breaks design principles. The reason that we've arrived here is that previously experiment names were less useful (e.g exp-43234) and the sha was thought of as the primary information identifier. The DVC team tried to solve the problem of not being able to navigate a lot of experiments by making the name easier to read/more memorable. The name as since become the primary identifier. However, the CLI still displays the information in the same way:

image

Studio have addressed this problem in the following way:

image

For more context please read #3619 (comment) & #3619 (comment) but the tl;dr is - we display this information in a lot of places, we should be standardising between the products, having the name first is a better option but a much larger change across multiple products.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, ok, makes sense! I didn't realize we were interested in matching order as well as focus across the extension! Hmmm, in that case, we could make secondary text first across all the rows:

image

Or just leave as is, up to you!

.experimentCellText {
@extend .cellContents;
display: flex;
flex-flow: row wrap;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to keep things stacked on top of each other, I think we need to give the rows more height. Things look pretty crowded vertically when they're wrapped:

image

@mattseddon
Copy link
Contributor Author

This and #3725 can wait until after the release of #3665 (0.8.0)

Base automatically changed from integrate-exp-show to main April 20, 2023 23:30
@mattseddon mattseddon force-pushed the match-studio-display branch from e34f534 to 74546a2 Compare April 20, 2023 23:34
@mattseddon mattseddon enabled auto-merge (squash) April 21, 2023 09:58
@mattseddon
Copy link
Contributor Author

Merging this as I think it is an improvement. We can iterate on it further as we get more feedback.

@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit d5a851f and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 81.8% (85% is the threshold).

This pull request will bring the total coverage in the repository to 94.7% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon merged commit da726ab into main Apr 21, 2023
@mattseddon mattseddon deleted the match-studio-display branch April 21, 2023 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

product PR that affects product

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants