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

Optimize & Consolidate Bundle Sidebar & Bundle View #4150

Merged
merged 17 commits into from
Jul 24, 2022

Conversation

leilenah
Copy link
Collaborator

@leilenah leilenah commented Jul 19, 2022

Reasons for making this change

This change optimizes the UI for viewing bundle data in the worksheet view as well as the standalone bundle view. Now both views leverage the same BundleDetail component, which has been improved from a UI perspective.

Related issues

#4131

Bundle Detail Demo

Screen.Recording.2022-07-22.at.12.38.04.PM.mov

Bundle View Demo

Screen.Recording.2022-07-22.at.12.48.13.PM.mov

Checklist

  • I've added a screenshot of the changes, if this is a frontend change
  • I've added and/or updated tests, if this is a backend change
  • I've run the pre-commit.sh script
  • I've updated docs, if needed

@leilenah leilenah linked an issue Jul 19, 2022 that may be closed by this pull request
@leilenah leilenah force-pushed the feature/4131-bundle-view-ui branch from 3622eae to 05fb8e9 Compare July 20, 2022 02:23
@leilenah leilenah marked this pull request as ready for review July 20, 2022 03:27
return Math.abs(Math.round(time));
}

getTimePassed() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we render the time field (which is time spent actually running)? Time since created is also meaningful, but sometimes not the thing you want. Maybe there's a way to render both? In general, maybe the thing to do is to render a total time and the time spent in the current stage?

Also, if we have a function for rendering a duration, could we factor this out? Ideally, it would match the one in formatting.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have removed the time-since-creation logic altogether. I feel like having that plus running time may be confusing to the user.

I have added times under the running state and preparing state. Appending times to the other states seemed like the logic required wouldn't be worth the payoff but plz lmk your thoughts.

Note: Times are also rendered in the More Details section for users to reference later if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Logically this is great. Would it be possible to move the times to the (right) side of the states, so that the state flowchart doesn't jump around when times are added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@percyliang certainly. Updated 👍

Screen.Recording.2022-07-24.at.3.41.49.PM.mov

Copy link
Collaborator

@percyliang percyliang left a comment

Choose a reason for hiding this comment

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

This is great - much cleaner and more uniform and helpful!

Just a few minor comments:

  1. The help icon next to each field is very faint.
  2. UUID maybe should go at the top before name.
  3. Bundle page could appear in the upper right corner like a NE arrow that looks like it'll expand things.
  4. Store should be after Size, logically
  5. Allowed Failed, Exclude Patterns can go after all the Request fields.
  6. The fields could better match the rendering when you create a new run (especially the grouping of the resources) - could we share a component?
  7. In the help text for State, we should write the state again (e.g., "staged: ...") so it's clear we're talking about the highlighted state. Maybe when you mouse over the states, they should show a description too.

@epicfaace
Copy link
Member

My feedback: #4153

@leilenah
Copy link
Collaborator Author

@percyliang

The fields could better match the rendering when you create a new run (especially the grouping of the resources) - could we share a component?

I have modified the labels to align with the labels in the new run form. I have also added field groupings.

In the help text for State, we should write the state again (e.g., "staged: ...") so it's clear we're talking about the highlighted state. Maybe when you mouse over the states, they should show a description too.

Note that since state_details are provided by the rest API for a single bundle, that bundle won't have the details for every state in the bundle lifecycle. It only has the details for the current state. I have added a tooltip for the current state in the lifecycle diagram, but the inactive states don't have tooltips. I could hardcode the definitions to achieve this, but I wanted to avoid that.

Copy link
Collaborator

@percyliang percyliang left a comment

Choose a reason for hiding this comment

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

Great work!

@mergify mergify bot merged commit f96a7fb into master Jul 24, 2022
@mergify mergify bot deleted the feature/4131-bundle-view-ui branch July 24, 2022 23:41
@teetone teetone mentioned this pull request Jul 26, 2022
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.

Optimize & Consolidate Bundle Sidebar & Bundle View
3 participants