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

[PLAY-1740] Fix Sort Function on Rails Advanced Table #3974

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

Tomm1128
Copy link
Contributor

@Tomm1128 Tomm1128 commented Dec 5, 2024

What does this PR do? A clear and concise description with your runway ticket url.
Fix a bug with the collapsible button and sort button where both functionalities happen when either button is clicked.

PLAY-1740

Screenshots: Screenshots to visualize your addition/change
There are no visual changes

How to test? Steps to confirm the desired behavior:

  1. Go to advanced table kit rails doc page - kits/advanced_table/rails
  2. Scroll down to Enable Sorting example on the page
  3. Clicking the sort button will refresh and sort the table.
  4. Clicking the collapsible button will expand the table and close the table if pressed again.
  5. Both of the buttons above should act independently of each other now.
  6. Check each collapsible button on each of the rails advanced tables to double check they still work with the most recent changes.

Checklist:

  • LABELS Add a label: enhancement, bug, improvement, new kit, deprecated, or breaking. See Changelog & Labels for details.
  • DEPLOY I have added the milano label to show I'm ready for a review.
  • TESTS I have added test coverage to my code.

@Tomm1128 Tomm1128 self-assigned this Dec 5, 2024
@Tomm1128 Tomm1128 added bug Fixes to issues discovered in Playbook (USED IN CHANGELOG) minor Semver Target labels Dec 5, 2024
@Tomm1128 Tomm1128 marked this pull request as ready for review December 5, 2024 16:35
@Tomm1128 Tomm1128 requested a review from a team as a code owner December 5, 2024 16:35
@Tomm1128 Tomm1128 added the milano 20 MAX - Deploy this PR to a review environment via Milano label Dec 5, 2024
@nidaqg
Copy link
Contributor

nidaqg commented Dec 6, 2024

This is awesome @Tomm1128 !! Love the simplicity of the solution! Tiny thing, did you look into that classname that is adding the hover scss? Can we get it so that hover only applies to the text and sort? If that hover is coming from the Table kit, we can always override and customize from the advanced Table scss!

Let me know if you want to pair on it at all or if that makes sense

@nidaqg
Copy link
Contributor

nidaqg commented Dec 6, 2024

This is awesome @Tomm1128 !! Love the simplicity of the solution! Tiny thing, did you look into that classname that is adding the hover scss? Can we get it so that hover only applies to the text and sort? If that hover is coming from the Table kit, we can always override and customize from the advanced Table scss!

Let me know if you want to pair on it at all or if that makes sense

Addressing this on a follow up so marking this approved! Great work here @Tomm1128 !

@nidaqg nidaqg added Code Approved Approved by a Playbook Admin and removed Needs Review labels Dec 6, 2024
@jasperfurniss jasperfurniss merged commit 088e9fd into master Dec 9, 2024
15 of 16 checks passed
@jasperfurniss jasperfurniss deleted the play-1740-rails-advanced-table-bug branch December 9, 2024 16:20
Copy link

github-actions bot commented Dec 9, 2024

You merged this pr to master branch:
- Ruby Gem: 14.10.0.pre.rc.9
- NPM: 14.10.0-rc.9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes to issues discovered in Playbook (USED IN CHANGELOG) Code Approved Approved by a Playbook Admin milano 20 MAX - Deploy this PR to a review environment via Milano minor Semver Target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants