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.

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

fix: Add data-bs-theme attribute for user dark/light modes #1782

Merged
merged 3 commits into from
Jul 4, 2023
Merged

fix: Add data-bs-theme attribute for user dark/light modes #1782

merged 3 commits into from
Jul 4, 2023

Conversation

jsit
Copy link
Contributor

@jsit jsit commented Jul 3, 2023 •
edited
Loading

Fixes #1774

Before

Screenshot 2023-07-03 at 12 29 08 PM

After

Screenshot 2023-07-03 at 12 26 22 PM

@jsit jsit mentioned this pull request Jul 3, 2023
Closed
SleeplessOne1917
SleeplessOne1917 requested changes Jul 3, 2023
View reviewed changes
Copy link
Member

@SleeplessOne1917 SleeplessOne1917 left a comment

Choose a reason for hiding this comment

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

The file you're importing dataBsTheme isn't included in the PR for some reason.

@jsit jsit requested a review from SleeplessOne1917 July 3, 2023 17:09
@snollygolly snollygolly mentioned this pull request Jul 3, 2023
Closed
4 tasks
@dessalines
Copy link
Member

dessalines commented Jul 3, 2023 •
edited
Loading

So confused rn.

  • Is this completely overriding everything in theme.tsx?
  • Why is all this necessary for one button?
  • Why is the data-bs attribute in use, I'd thought we removed all bootstrap javascript?
  • Why is bootstrap-specific javascript necessary for that button styling?

Shouldn't just doing the correct bootstrap class name be enough? https://getbootstrap.com/docs/5.3/forms/form-control/#file-input

@jsit
Copy link
Contributor Author

jsit commented Jul 3, 2023 •
edited
Loading

This isn't Bootstrap JS; it's our own JS to add an additional attribute to a high-level element.

There are a few things in Bootstrap CSS that depend on this attribute selector; for instance if you look in litely.css you'll see:

[data-bs-theme=dark] .accordion-button::after {
  --bs-accordion-btn-icon: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16' fill='%23f7a278'%3e%3cpath fill-rule='evenodd' d='M1.646 4.646a.5.5 0 0 1 .708 0L8 10.293l5.646-5.647a.5.5 0 0 1 .708.708l-6 6a.5.5 0 0 1-.708 0l-6-6a.5.5 0 0 1 0-.708z'/%3e%3c/svg%3e");
  --bs-accordion-btn-active-icon: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16' fill='%23f7a278'%3e%3cpath fill-rule='evenodd' d='M1.646 4.646a.5.5 0 0 1 .708 0L8 10.293l5.646-5.647a.5.5 0 0 1 .708.708l-6 6a.5.5 0 0 1-.708 0l-6-6a.5.5 0 0 1 0-.708z'/%3e%3c/svg%3e");
}

Background SVG images can't have their colors modified by CSS, so the SVG markup itself has to be changed.

This affects not just file upload buttons, but select menus as well. Here it is without data-bs-theme:

Screenshot 2023-07-03 at 5 09 26 PM

And with:

Screenshot 2023-07-03 at 5 09 55 PM

More info here: https://getbootstrap.com/docs/5.3/customize/color-modes/

@dessalines
Copy link
Member

Ah my bad. I'm used to seeing those data- selectors only for bootstrap javascript.

@dessalines dessalines merged commit b1292b9 into LemmyNet:main Jul 4, 2023
@ghost
Copy link

ghost commented Jul 4, 2023

@jsit It seems you have missed the compact theme 🙈

Screenshot 2023-07-04 at 18 32 42 Tested on 0.18.1-RC10

@jsit
Copy link
Contributor Author

jsit commented Jul 4, 2023

@jsit It seems you have missed the compact theme 🙈

Thank you! There's a fix for this already in the works.

jsit added a commit to jsit/lemmy-ui that referenced this pull request Jul 4, 2023
@jsit
Merge remote-tracking branch 'lemmy/main' into fix/fix-toggle-radio-f…
e5b5c08 
…ocus-1772

* lemmy/main: (25 commits)
  v0.18.1-rc.10
  Attempt to fix inability to logout from some instances (subdomains) (LemmyNet#1809)
  feat(theme): Vaporwave (LemmyNet#1682)
  fix: Revert smaller text size (LemmyNet#1795)
  Updated the regex for isAuthPath to reduce false positive hits (LemmyNet#1806)
  fix: Add focus border to markdown toolbar buttons
  fix: Add data-bs-theme attribute for user dark/light modes (LemmyNet#1782)
  v0.18.1-rc.9
  fix: Fix comment collapse and vote buttons not having focus style (LemmyNet#1789)
  Add missing modlog reasons (LemmyNet#1787)
  Fix search page breaking on initial load when logged in (LemmyNet#1781)
  feat: Add PR template (LemmyNet#1785)
  v0.18.1-rc.8
  Fix profile loading spinner
  fix: Break text on post titles so long words don't overflow
  fix: Move getRoleLabelPill to the only component that uses it
  fix: Remove unused hasBadges() function
  fix: Fix badge alignment and break out into component
  fix: Fix up filter row gaps and margins a little
  fix: Fix heading levels
  ...
@OpaqueReptile
Copy link

This seems to cause odd style mixing when the instance is set to the lighty theme, and the user is set to browser default+dark mode browser/os. I am now seeing this upon upgrading to 0.18.1:

image

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

@SleeplessOne1917 SleeplessOne1917 SleeplessOne1917 approved these changes

@dessalines dessalines dessalines approved these changes

@alectrocute alectrocute Awaiting requested review from alectrocute

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upload image buttons use white theme even on a dark one
4 participants
@jsit @dessalines @OpaqueReptile @SleeplessOne1917