8000 Add JS for responsive tables in long text fields by markconroy · Pull Request #771 · localgovdrupal/localgov_base · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Ad 8000 d JS for responsive tables in long text fields #771

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 12 commits into
base: 2.x
Choose a base branch
from
Open

Add JS for responsive tables in long text fields #771

wants to merge 12 commits into from

Conversation

markconroy
Copy link
Member
@markconroy < 8000 a class="author Link--primary text-bold css-overflow-wrap-anywhere " show_full_name="false" data-hovercard-type="user" data-hovercard-url="/users/markconroy/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="/markconroy">markconroy commented May 14, 2025 •
edited
Loading

Closes #759

What does this change?

  • Finds tables in long text fields, clones the content of the table, and then places it inside a div with a class of lgd-responsive-table, so we can then use overflow-x: auto on these containers.
  • Adds a theme setting - turned on by default for new installs - to allow existing sites use this feature

How to test

  • Go to your theme settings page, check the checkbox for 'Make WYSIWYG tables responsive'
  • Create a table in a long text field
  • Reduce the size of your screen to mobile size
  • Check that the table can be scrolled horizontally, but the page does not scroll with it

Caveat

Do we want to use a theme setting to enable this?

Related PR

#648

^ That PR is for the table field tables rather than wysiwyg tablets iirc.

Thanks to Big Blue Door for sponsoring my time to work on this.

@markconroy markconroy requested review from finnlewis and msayoung May 14, 2025 17:11
@willguv
Copy link
Member
willguv commented May 15, 2025

@markconroy this feels like something could add to the product newsletter too. Do councils just update localgov_base?

Thanks for doing this

@willguv willguv moved this to In Progress - Core team in 2025 Prio 8000 rities May 15, 2025
@msayoung
Copy link
Member

Hey @markconroy - this works fine.

I'm just wondering if we should use something like this instead ?
https://www.drupal.org/project/responsive_tables_filter which allows for some options on how to manage responsive tables.

What do you think?

@markconroy
Copy link
Member Author

I wasn't aware of that module. Let's chat about it in MT.

I have a feeling wiht that module, it's changing the design/layout of tables pretty drastically, so maybe it would be worth putting that module in "as well as" this PR. Then sites can decide if they want the full-on experience of that module, or something similar like this PR per site.

@msayoung
Copy link
Member

I won't make it to MT today, dentist calls.

This module can be set to default to the same behaviour you've made, with the option to override it on a case by case basis. That might be the safest option.

The downside is that it only works if there's a table header set, which there should be anyway.

There is also this module https://www.drupal.org/project/responsive_table_filter which seems to do the same as yours, if we want to stick to contrib ? I haven't looked at how it does it though.

msayoung
msayoung approved these changes May 29, 2025
View reviewed changes
Copy link
Member
@msayoung msayoung left a comment

Choose a reason for hiding this comment

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

Discussed with Mark and decided to go with this approach for now, and people can not enable this and use whichever other module they like.

@finnlewis finnlewis changed the base branch from 1.x to 2.x June 3, 2025 11:39
@msayoung msayoung self-requested a review June 3, 2025 11:41
@finnlewis
Copy link
Member

We want this to merge into the 2.x branch first, then backport to the 1.x.

So maybe we recreate the merge requests branching off the 2.x branch.

Or resolve the merge conflicts.

@tonypaulbarker will take a look :)

@finnlewis finnlewis requested a review from tonypaulbarker June 3, 2025 11:41
millnut
millnut requested changes Jun 3, 2025
View reviewed changes
Copy link
Member
@millnut millnut left a comment

Choose a reason for hiding this comment

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

Also looks like this PR has conflicts

localgov_base.theme
'#title' => t('Make WYSIWYG tables responsive.'),
'#default_value' => theme_get_setting('localgov_base_responsive_wysiwyg_tables'),
'#description' => t('Places tables inserted via WYSIWYG into a responsive container, so they can scroll horizontally on small screens.'),
'#default_value' => theme_get_setting('localgov_base_responsive_wysiwyg_tables') ? theme_get_setting('localgov_base_responsive_wysiwyg_tables') : FALSE,
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate #default_value with line 105 above

Copy link
@tonypaulbarker tonypaulbarker Jun 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@markconroy @millnut line 107 has some additional logic compared to line 105. Presume we want line 107. "If we have this setting, use the setting, otherwise false" ?

@tonypaulbarker
Copy link
tonypaulbarker commented Jun 9, 2025 •
edited
Loading

Some conflicts in grid.css

I assume net changes to this file should be zero.

I think that we want a grid of '12' columns, but just to be sure:

.lgd-row {
  display: grid;
  gap: var(--grid-column-spacing);
  grid-template-columns: repeat(12, 1fr);
}

or

.lgd-row {
  display: grid;
  gap: var(--grid-column-spacing);
  grid-template-columns: 1fr;
}

.lgd-row__one-quarter,
.lgd-row--quarters > *,
.lgd-row__one-third,
.lgd-row--thirds > *,
.lgd-row__one-half,
.lgd-row--halves > *,
.lgd-row__two-thirds,
.lgd-row__three-quarters,
.lgd-row__full {
  grid-column: span 12;
  width: 100%;
}

or

.lgd-row__one-quarter,
.lgd-row--quarters > *,
.lgd-row__one-third,
.lgd-row--thirds > *,
.lgd-row__one-half,
.lgd-row--halves > *,
.lgd-row__two-thirds,
.lgd-row__three-quarters,
.lgd-row__full {
  width: 100%;
  grid-column: span 1;
}

@tonypaulbarker
Copy link

Conflicts should be well now. Please review comments, review code and re-test this works as expected.

@msayoung
Copy link
Member

Seems to work fine @tonypaulbarker , but the are some failing tests. Can you have a look at those?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@millnut millnut millnut requested changes

@finnlewis finnlewis Awaiting requested review from finnlewis

@msayoung msayoung Awaiting requested review from msayoung

@tonypaulbarker tonypaulbarker Awaiting requested review from tonypaulbarker

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
2025 Priorities
Status: In Progress - Core team
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Table added through the WYSIWYG breaks content reflow
6 participants
@markconroy @willguv @msayoung @finnlewis @tonypaulbarker @millnut
0