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][Dashboard] Fix Place Group Table table cells overflow #47323

Merged

Conversation

MortalHappiness
Copy link
Member

@MortalHappiness MortalHappiness commented Aug 24, 2024

Why are these changes needed?

See the description in the corresponding issue for details.

In this PR, the ID will never be cut, and the bundles will display differently according to the array length.

Before:

image

After:

  • length is 0 (show -)

image

  • length is 1 (show directly)

image

  • length >= 2 (click to expand)

image
image

Related issue number

Closes: #47315

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@MortalHappiness
Copy link
Member Author

@kevin85421 PTAL

@MortalHappiness MortalHappiness force-pushed the bugfix/#47315-dashboard-text-clipped branch from e2c0099 to 320d8df Compare August 24, 2024 20:09
@kevin85421 kevin85421 requested a review from alanwguo August 26, 2024 17:44
@kevin85421
Copy link
Member

@alanwguo would you mind taking a look? Thanks!

@anyscalesam anyscalesam added dashboard Issues specific to the Ray Dashboard triage Needs triage (eg: priority, bug/not-bug, and owning component) labels Aug 26, 2024
Copy link
Contributor

@alanwguo alanwguo left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. The approach looks good.

@scottsun94 , can you review the UX design?

overflow: "hidden",
textOverflow: "ellipsis",
width: "100%",
whiteSpace: "nowrap",
Copy link
Contributor

Choose a reason for hiding this comment

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

this rowStyles is it used by multiple tables? Have you verified if other tables don't break their layout due to this change?

Copy link
Member Author

@MortalHappiness MortalHappiness Sep 8, 2024

Choose a reason for hiding this comment

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

This style is used in
image

So change this style can fix them all.

Before:
image
image
image

After:

image
image
image

@scottsun94
Copy link
Contributor

Thanks for the PR! RE the design, can we make it a bit simpler?

  1. first, we shouldn't truncate the text when it doesn't even fully use the cell's width
  2. when it gets too long, can we just truncate it and add an arrow to expand it? The design in the PR seems to take a lot of space and requires the user to click first to see anything?
Screenshot 2024-09-04 at 2 53 09 PM

@MortalHappiness MortalHappiness force-pushed the bugfix/#47315-dashboard-text-clipped branch from 2fd994d to e5ac7f4 Compare September 8, 2024 14:06
@MortalHappiness
Copy link
Member Author

@scottsun94 I've updated the design. See the following video for details.

pr47323.mp4

@scottsun94
Copy link
Contributor

@MortalHappiness Cool. LGTM. Thanks!

@kevin85421
Copy link
Member

cc @alanwguo does the code change look good to you?

Copy link
Contributor

@alanwguo alanwguo left a comment

Choose a reason for hiding this comment

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

thanks, lgtm!

@MortalHappiness MortalHappiness force-pushed the bugfix/#47315-dashboard-text-clipped branch from e5ac7f4 to c645923 Compare October 16, 2024 14:08
@jjyao jjyao added the go add ONLY when ready to merge, run all tests label Nov 6, 2024
@jjyao jjyao enabled auto-merge (squash) November 6, 2024 23:15
@jjyao jjyao merged commit ac84104 into ray-project:master Nov 7, 2024
7 checks passed
JP-sDEV pushed a commit to JP-sDEV/ray that referenced this pull request Nov 14, 2024
mohitjain2504 pushed a commit to mohitjain2504/ray that referenced this pull request Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dashboard Issues specific to the Ray Dashboard go add ONLY when ready to merge, run all tests triage Needs triage (eg: priority, bug/not-bug, and owning component)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dashboard] Placement group table reserved resources text clipped
6 participants