-
Notifications
You must be signed in to change notification settings - Fork 2
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
[SAGE-757] Table - responsive updates #1573
base: develop
Are you sure you want to change the base?
Conversation
eda642a
to
3750449
Compare
dc30519
to
086ddaa
Compare
7e59ad5
to
56af73e
Compare
086ddaa
to
0411d30
Compare
0411d30
to
50f2118
Compare
50f2118
to
3263a9a
Compare
table_classlist << " sage-table--condensed" if table.condensed | ||
table_classlist << " sage-table--has-leading-input" if table.has_leading_input | ||
table_classlist << " sage-table--has-menu-options" if table.has_menu_options | ||
table_classlist << " sage-table--stack" if table.responsive_stack |
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.
Will need to clarify with design here:
I've created a new prop responsive_stack
rather than repurposing is_responsive
. This is all under the assumption that we want to maintain maximum backwards compatibility with overflow/scrolling tables since the responsive stack layout conditions are somewhat stringent. Retrofitting all current table uses is not likely to be a trivial effort.
tableHeadings.push(label); | ||
}) | ||
|
||
rows.forEach(row => { |
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.
Using multiple nested loops to dig into the table markup. Probably not ideal, but I'm not sure we have much of an alternative. Performance impact should be minimal with the majority of cases rendering paginated results.
<%= "sage-table--condensed" if component.condensed %> | ||
" | ||
<table | ||
class="<%= sage_table_classes(component) %>" |
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.
Moved CSS class output logic to sage_table_helper
98a2550
to
79a813a
Compare
034eb39
to
f783196
Compare
refactor JS & css
694e1d8
to
50aee97
Compare
11dde9c
to
48f6933
Compare
81a9318
to
da9b7c3
Compare
7caaef2
to
3359401
Compare
Description
Repurposes table markup for a "stacked" visual layout on small screens.
display
. Each defaultrole
has been added back in with JS.th
) text into each individual celltd
asspan
elements. The actual table headers are visually hidden but remain readable for screen readers, keeping existing semantics/relationship with cells intact.Requirements:
responsive_stack
property must be enabledis_responsive
property can still be set independently, and will take effect on larger screen sizes OR when mobile stack content overflows its containerCurrent assumptions:
max-width
set on truncated text causes issue with cell width layoutTODOs
headers
schema overridesage_table_for
- requires nested string(s)/schemaScreenshots
sage_table_for
Testing in
sage-lib
Visit the table view and verify that the mobile view aligns with the design spec
Testing in
kajabi-products
Related
Closes SAGE-757