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

Table: Ensure responsive: false turns off responsive features #1611

Merged
merged 2 commits into from
Oct 6, 2022

Conversation

philschanely
Copy link
Contributor

@philschanely philschanely commented Oct 3, 2022

Description

This PR patches some faulty logic within SageTable to ensure that when users set responsive: false the responsive features are indeed disabled.

This fix relates to how component.responsive.present? evaluates an explicit false value. We want the tables to be responsive by default. Users should be able to override this by setting responsive: false. This table shows how the prior logic was evaluating values provided compared to the new logic.

Value provided Original is_responsive = component.responsive.present? ? component.responsive : true New is_responsive = component.responsive != false
None/nil 👍 true 👍 true
true 👍 true 👍 true
false true 👍 false

The issue seemed to be that when false was provided component.responsive.present? still resolved to false so the ternary would return true where we though it would return component.responsive.

Screenshots

Before After
responsive: true or not set 👍 Screen Shot 2022-10-03 at 12 04 42 PM 👍 Screen Shot 2022-10-03 at 12 04 42 PM
responsive: false Screen Shot 2022-10-03 at 12 04 42 PM 👍 Screen Shot 2022-10-03 at 12 02 48 PM

Testing in sage-lib

See http://localhost:4000/pages/component/table?tab=preview for no regressions.

Testing in kajabi-products

  1. (LOW) Fix SageTable so that responsive: false has the intended effect of disabling sideways scrolling.

Related

https://kajabi.atlassian.net/browse/DSS-158

@philschanely philschanely self-assigned this Oct 3, 2022
@philschanely
Copy link
Contributor Author

@ju-Skinner would you be willing to give this an early review? Any other ideas for how to make this most readable?

@philschanely
Copy link
Contributor Author

@ju-Skinner thanks for early feedback through Slack! 🙌

@philschanely philschanely marked this pull request as ready for review October 4, 2022 16:18
@philschanely philschanely requested review from a team and ju-Skinner October 4, 2022 16:18
Copy link
Member

@pixelflips pixelflips left a comment

Choose a reason for hiding this comment

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

LGTM! 👍🏼

@philschanely philschanely requested a review from a team October 4, 2022 16:19
@pixelflips pixelflips requested a review from a team October 4, 2022 16:19
Copy link
Member

@teenwolfblitzer teenwolfblitzer left a comment

Choose a reason for hiding this comment

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

👍 LGTM! Interestingly, while testing this in KP it seems the main Offers table overflows at certain screen sizes. Appears to be an existing issue.

@philschanely philschanely merged commit 2531bd9 into develop Oct 6, 2022
@philschanely philschanely deleted the DSS-158/pjs-responsive-table-patch branch October 6, 2022 13:50
@philschanely philschanely mentioned this pull request Oct 6, 2022
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