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

[DT-997] Move the Donor Size Header in DUOS further left so it aligns with the rest of the table #2735

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

cinyecai
Copy link
Contributor

Addresses

https://broadworkbench.atlassian.net/browse/DT-997

Summary

Increase the spacing between the Donor Size and the Data Location columns in the Dataset table in the Data Library
image


Have you read Terra's Contributing Guide lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@cinyecai cinyecai requested a review from a team as a code owner November 21, 2024 15:43
@cinyecai cinyecai requested review from rushtong and fboulnois and removed request for a team November 21, 2024 15:43
Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

👍🏽

Copy link
Member

@pshapiro4broad pshapiro4broad left a comment

Choose a reason for hiding this comment

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

This looks OK but the underlying problem that I see is that this doesn't add padding between elements, so a "correct" appearance is still dependent on how a user sizes their window. For example, by resizing the window, you can create a state where DATASET NAME becomes too close to STUDY NAME in the dataset view.

It would be better if there's an overall way to specify between-cell spacing for the table, so we don't have to approximate the correct behavior by making columns wider.

donorSize: '7%',
donorSize: '10%',
Copy link
Member

Choose a reason for hiding this comment

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

Before the total %s equalled 100 when added up (25 + 10 + 10+ 15 + 7 + 13 + 10). Now it's 103%. Is that correct? If this % goes up, maybe another needs to go down?

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 thought that too, but it doesn't seem to cause any weird behavior in the rendering of the table itself, but I'll look more into it

Copy link
Contributor

Choose a reason for hiding this comment

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

Widths can be > 100%. It can sometimes lead to overflow issues.

@cinyecai
Copy link
Contributor Author

This looks OK but the underlying problem that I see is that this doesn't add padding between elements, so a "correct" appearance is still dependent on how a user sizes their window.

This is a good point - I checked and it looks like there's padding build into the cell styling, but not the header styling. Adding a right margin property in the header styling helps with the window size problem a little by forcing text wrapping before the headers get too close to each other, but eventually the whole page looks wonky if your window is too small, which I think is a larger problem

@cinyecai cinyecai merged commit f196234 into develop Nov 22, 2024
9 checks passed
@cinyecai cinyecai deleted the cc-dt-997-move-donor-size-header branch November 22, 2024 15:48
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.

3 participants