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

[YUNIKORN-2829] Improve table layout for Applications #214

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

blueBlue0102
Copy link
Contributor

What is this PR for?

Improve table layout for Applications.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

What is the Jira issue?

YUNIKORN-2829

How should this be tested?

Screenshots (if appropriate)

Before:
before

After:
after

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

We should still center the column values (as the headers already are).

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 35.71%. Comparing base (d082b14) to head (19e36ef).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #214   +/-   ##
=======================================
  Coverage   35.71%   35.71%           
=======================================
  Files           2        2           
  Lines          56       56           
=======================================
  Hits           20       20           
  Misses         33       33           
  Partials        3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@blueBlue0102
Copy link
Contributor Author

Is this what you had in mind?

text-align center 0
text-align center
text-align center 2

or this one:
0
1
2

@craigcondit
Copy link
Contributor

Is this what you had in mind?

Neither. Simply ensure that each cell has the same justification (left for column 1, centered for others). There are strange offsets that look out of place in every example you've shown so far.

@blueBlue0102
Copy link
Contributor Author

Before this PR, all the values were left-aligned in every column.
任何修改以前67%
任何修改以前100%
任何修改以前125%
任何修改以前

I can fix the alignment to match what you suggested - left for the first column and centered for the rest.

@chenyulin0719
Copy link
Contributor

Hi @blueBlue0102 , Could you resolve the conflicts?

@blueBlue0102
Copy link
Contributor Author

Before this PR, all the values were left-aligned in every column. 任何修改以前67% 任何修改以前100% 任何修改以前125% 任何修改以前

I can fix the alignment to match what you suggested - left for the first column and centered for the rest.

Hi @craigcondit, do you have any thoughts on this approach?

@craigcondit
Copy link
Contributor

I can fix the alignment to match what you suggested - left for the first column and centered for the rest.

Hi @craigcondit, do you have any thoughts on this approach?

That should be fine. Please fix the merge conflicts as well.

@craigcondit
Copy link
Contributor

@blueBlue0102 did you mean to revert everything?

@blueBlue0102
Copy link
Contributor Author

@craigcondit Because there are too many conflicts, I decide to revert it. I am still working on this.

@blueBlue0102 blueBlue0102 marked this pull request as draft November 22, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants