-
Notifications
You must be signed in to change notification settings - Fork 12
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
va-table: Adds ability to arbitrarily right-align columns #1486
base: main
Are you sure you want to change the base?
Conversation
packages/web-components/src/components/va-table/va-table-inner/va-table-inner.scss
Outdated
Show resolved
Hide resolved
/** | ||
* A comma-separated, zero-indexed string of which columns, if any, should be right-aligned | ||
*/ | ||
"rightAlignCols"?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was my belief, shared by Micah, that having a single prop at the va-table
component level was going to be preferable to asking developers to add a prop to every row or every desired span. If we did it at the row level, it would still need to be a complex prop to define which spans within the row needed to be right-aligned. If we did it at the span level, devs would either need to manually or programmatically add the .vads-u-text-align--right
class to just the spans they wanted right-aligned, increasing the chances of error and just generally making for an unpleasant developer experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I feel like I would want more flexibility to be able to add a styling class where I needed it.
Is it always the case that all cells in a column must be right aligned? Is there a situation that some cells would not be right aligned (mixed alignment)?
Sorry for all the questions if there was already conversation about this. Less code is less bugs for us. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, I'm ultimately asking if this can just be accomplished with guidelines especially if it's going to get complicated with mixed cell alignments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about mixed cell alignments, though I would suspect that'd make for a very unattractive-looking table. The ticket just called for alignment options for columns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If anyone wanted to manually align individual cells, they'd still have the option to add whatever classes they wanted to the individual spans. This just gives a convenient way to right-align entire columns.
packages/web-components/src/components/va-table/va-table-inner/va-table-inner.scss
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/va-table/va-table-inner/va-table-inner.tsx
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/va-table/va-table-inner/va-table-inner.tsx
Outdated
Show resolved
Hide resolved
packages/web-components/src/components/va-table/va-table-inner/va-table-slot.css
Outdated
Show resolved
Hide resolved
@danbrady Fixed! |
Chromatic
https://2846-right-alignment-for-tables--65a6e2ed2314f7b8f98609d8.chromatic.com
Description
Closes #2846
QA Checklist
Screenshots
Acceptance criteria
Definition of done