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

Views dropbuttons have bad widths and text alignment in Basis theme #6180

Closed
bugfolder opened this issue Jul 23, 2023 · 10 comments · Fixed by backdrop/backdrop#4484
Closed

Comments

@bugfolder
Copy link

Description of the bug

Views dropbuttons in the Basis theme have bad widths and text alignment. The width of the dropbutton changes when clicked on; it can extend outside of its enclosing table cell; and the text alignment of the individual entries is right-justified (should be left).

Steps To Reproduce

To reproduce the behavior:

Install the attached View, which creates two otherwise identical pages with paths dropbutton-test and admin/dropbutton-test. The first will render with Basis theme, the second with Seven theme (this one renders correctly).

Actual behavior

Path admin/dropbutton-test, in Seven: this renders correctly.

seven

Path dropbutton-test, in Basis. You can see all of the problems.

basis

Expected behavior

Basis should render the dropbuttons as nicely as Seven.

Additional information

A difference between Seven and Basis is that Seven contains a JS behavior, Backdrop.behaviors.sevenDropButtonWidths, in file seven/js/script.js, that sets the min-width of the dropbutton and its parent element, which addresses the changing width and extension outside of the table column. There is also additional CSS in Seven that takes care of the text alignment. So the apparent solution would be to copy the JS and CSS (suitably modified) from Seven into Basis.

The existing issue #6010 mentions several D* issues that apply to dropbuttons, and one of them, https://www.drupal.org/project/drupal/issues/3168326, shows similar width-changing behavior.

This is a View that demos the two pages. Drop the .txt extension, add it to staging config and synchronize.
views.view.dropbutton_test.json.txt

@bugfolder
Copy link
Author

bugfolder commented Jul 25, 2023

In thinking about this further, things like styling the button content in all-caps, setting font size, left- or right-justifying etc., are the responsibility of the theme CSS. But setting the min-widths so that things don't jump around is basic functionality that should be common to any site that uses dropbuttons. So it seems to make more sense to move the JS script out of Seven and into Backdrop Core. That would make the dropbutton at least functional in Basis (and other themes that don't already re-implement the Seven JS).

But maybe add a little touch-up CSS to Basis so that the button text is appropriately justified.

@klonos
Copy link
Member

klonos commented Jul 26, 2023

That does make sense to me too @bugfolder 👍🏼

@stpaultim
Copy link
Member

Any relationship to: #2420

@jenlampton
Copy link
Member

In this particular instance, I think that the risk of this change (moving JS from Seven to System, and renaming the behavior) would have a very low chance of affecting anyone negatively - and a very high chance of fixing this bug. I'm in favor of fixing this.

@bugfolder
Copy link
Author

I've filed a PR on this, just moving the JS from Seven into core, but not making any CSS changes. I did have to add detection of the font since other themes will use different fonts from Seven.

I also tested this change while leaving the JS in Seven, to see if having two copies of the JS caused any problems. There were no visible problems. That suggests that if someone did implement their own min-width-setting code in their theme, if they did it by copying the code from Seven, then it's probably compatible with this change.

@avpaderno
Copy link
Member

avpaderno commented Jul 29, 2023

I added the view to the preview site importing the configuration attached to this issue.
The drop-buttons work fine with the admin theme and with the Basis theme. The only difference I noticed, between the admin theme and the Basis theme, is that the drop-button links are rendered underlined, but that happens with all the links rendered in the Basis theme. I mean that the only difference I noticed is not caused by the PR, which (rightly) does not change the theme CSS styles.

screenshot

@bugfolder
Copy link
Author

@kiamlaluno, thanks for your review and testing. Are you comfortable enough with your testing to add the "pr - works for me" and (after code review) "pr - ready to be committed" labels?

@avpaderno
Copy link
Member

I am going to check the site preview on Vivaldi and Google Chrome. (First I checked the preview with Firefox.) If I do not see anything different, I will change the labels.

@avpaderno
Copy link
Member

I checked the file changes: The JavaScript behavior has been moved from a theme file to core/misc/dropbutton.js, as expected.
I checked the preview site with Firefox Developer Edition, Vivaldi, and Google Chrome. In none of them, I found anything wrong in the views I added to the preview site as per my previous comment.

(I hope I changed the labels correctly.)

@laryn laryn added this to the 1.25.2 milestone Aug 16, 2023
backdrop-ci pushed a commit to backdrop/backdrop that referenced this issue Aug 17, 2023
laryn pushed a commit to backdrop/backdrop that referenced this issue Aug 17, 2023
@laryn
Copy link
Contributor

laryn commented Aug 17, 2023

Thank you @bugfolder and @kiamlaluno -- merged into 1.x and 1.25.x!

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

Successfully merging a pull request may close this issue.

6 participants