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

Stop using JavaScript to hide columns #5141

Closed
wants to merge 1 commit into from

Conversation

rfultz
Copy link
Contributor

@rfultz rfultz commented Mar 29, 2022

Summary

Required reviewers

  • front-end
  • UX
  • someone else to help check possible weirdness

Impacted areas of the application

Potentially all datatables

Screenshots

image

left: initial load;
right: after opening a details panel on the smallest browser width

Related PRs

None

How to test

How to replicate

  • go to a datatables page
  • shrink the browser window to the smallest width
  • reload the page
  • right-click the first Source name listed and Inspect
  • see how there are seven <td>? That quantity is what we're watching
  • back to the main window
  • close the filters (they're just in the way)
  • click one of the ⊕ in the right column of the table
  • see how now there are fewer <td>?
  • now expanding the browser window won't restore those <td> because they no longer exist
  • that's what this PR is trying to fix

Testing this PR

  • pull the branch
  • maybe npm i but likely not
  • npm run build (or just npm run build-js)
  • ./manage.py runserver
  • go to the datatables page linked in 'How to replicate'
  • follow the replication steps
  • now, expanding the browser window after having opened and closed the ⊕ panel should show the same columns as a wide-layout page load

Side effects

  • Does this break anything on other datatables pages?
  • This won't be a fun test.
  • We'll need to test opening and close panels on different width on every template.
  • Why was api.columns…visible() being used at all? Was it just left-over from before the hide-panel-* classes were implemented? There's a comment in tables.js: // TODO figure way to share these values with CSS.

Next steps

If approved, remove the commented code from tables.js

Copy link
Contributor

@johnnyporkchops johnnyporkchops left a comment

Choose a reason for hiding this comment

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

@rfultz I think this code was to hide some columns to keep the details panel from getting pushed under the table at a certain desktop breakpoint. But then on mobile (where thats not needed because of the overlay behavior), the columns do not reappear after the panel is closed. Is there a better way to solve that display problem or just remove the classes upon closing panel ?

Here is what I am seeing with this code removed on the PR branch ( I don't think you would see it if the filter panel is collapsed):

details_panel_stacking_below_table

Copy link
Contributor

@JonellaCulmer JonellaCulmer left a comment

Choose a reason for hiding this comment

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

Everything is working as it should again - no disappearing columns at the smallest widths! Thank you, @rfultz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: On mobile, after opening details panel, columns disappear from view
3 participants