-
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
Open
Andrew565
wants to merge
6
commits into
main
Choose a base branch
from
2846-right-alignment-for-tables
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+106
−15
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
063bbbf
WIP Adding rightAlignCols prop
Andrew565 f891059
va-table: Adds the ability to right-align arbitrary columns
Andrew565 4c353d0
Replacing right-align class
Andrew565 b27fbf3
Improvements to stylesheets and column handling
Andrew565 f861d65
Removing right-align from table headers
Andrew565 1c652db
Fix for text alignment of headers when stacked
Andrew565 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is it possible to do this without having a "complex" data string to parse the column values. Like could they just add a prop (boolean) to
va-table-row
for each column they want to be right aligned?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.