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

feat(ui): rework theme switching and make navbar responsive #1898

Draft
wants to merge 16 commits into
base: dev
Choose a base branch
from

Conversation

mrskycriper
Copy link
Contributor

@mrskycriper mrskycriper commented Feb 1, 2024

TODO

  • replace react-css-theme-switcher with bootstrap color modes
  • add auto theme and update theme-switcher.ts
  • make navbar collapse without overflowing
  • update LocalePicker design and make more responsive
  • update StartStopJoinButton design and make more responsive
  • move isIframe() and custom header name away from navbar into the settings as information
  • resolve dropdown positioning issue
  • await new appstack release for proper theme support
  • code cleanup

Reasons

  • Responsive navbar - pretty obvious, currently in reacts to smaller screen size by overflowing, making it difficult to use on mobile devices and smaller screens in general. Especially if your native language has long translations.
  • Navbar design overall - wanted to standardize the look and make it cleaner in both horizontal and especially vertical states.
  • Theme switching - might be controversial, but automatic theme is a must have feature. And bootstrap now provides a cleaner mechanism to switch themes. Though the custom styles must be migrated as scss overrides.
  • LocalePicker - wasn't responsive enough and also some flags had poor legibility on different backgrounds.
  • StartStopJoinButton - long device names will absolutely break the navbar so now they are just marked active in the list.
  • Move isIframe() - typical external address looks like this: dsfbndsgiudsfgsdfhgfshdsbfkbdgubsdusdsdugb.domain.name and overflows the navbar. I don't think it's that important to see here. For responsiveness it would need to be capped at e certain length, which makes it useless and look worse than simple and clean Zigbee2MQTT.

Demo

Link to video demo

Problems

As mentioned in #1897 I currently can't add the necessary translation keys for themes.

@mrskycriper mrskycriper marked this pull request as draft February 1, 2024 15:03
@nurikk
Copy link
Owner

nurikk commented Feb 1, 2024

Hey mate! Thanks for your interest in helping! I really appreciate it. I only want to let you know that ligh.css and dark.css comes from paid bootstrap theme which I acquired for this project. I have sources in a separate private repo, I can share access with you if you’re interested

@mrskycriper
Copy link
Contributor Author

Hey mate! Thanks for your interest in helping! I really appreciate it. I only want to let you know that ligh.css and dark.css comes from paid bootstrap theme which I acquired for this project. I have sources in a separate private repo, I can share access with you if you’re interested

Oh, that is why it looks a bit different than standard bootstrap. Are those sources any different from the included minified ones and are there any updates to this theme? Because it looks like they are out of date with bootstrap version.

@nurikk
Copy link
Owner

nurikk commented Feb 1, 2024

Hey mate! Thanks for your interest in helping! I really appreciate it. I only want to let you know that ligh.css and dark.css comes from paid bootstrap theme which I acquired for this project. I have sources in a separate private repo, I can share access with you if you’re interested

Oh, that is why it looks a bit different than standard bootstrap. Are those sources any different from the included minified ones and are there any updates to this theme? Because it looks like they are out of date with bootstrap version.

I've sent you an invite to the private repo with theme sources and downloaded latest version (released 5 days ago). We can modify it and release compiled css files, but we're not allowed to include source files into out open source project

This is theme official web page https://themes.getbootstrap.com/product/appstack-responsive-admin-template/

@mrskycriper
Copy link
Contributor Author

Could you look at this issue please? I have some translation to submit and will also need to add new keys there for themes.

@mrskycriper
Copy link
Contributor Author

@nurikk I've spent a lot of time, trying to re-integrate the appstack theme, but currently it's a bit of a disaster.

First of all, appstack does not conform to the standard and both dark and light theme were set inernally as data-bs-theme=light. I've manually corrected this and merged two theme files together, as appstack doesn't even acknowledge the possibility of dynamic theming.

And after all those fixes it worked, almost. It totally breaks dropdowns placement on the screen.

Снимок экрана 2024-02-03 в 01 12 01

With default bootstrap theme everything works as intended.

I'll try and mimic the look by just using variable overrides on default bootstrap theme.

@mrskycriper
Copy link
Contributor Author

I’m just realized that this would not be allowed as I’ll have to include most of the theme sources as slightly modified overrides. I don’t really know, what to do with this. Personally I’d just used slightly modified default theme but I don’t know, how acceptable this would be in this project.

@nurikk
Copy link
Owner

nurikk commented Feb 4, 2024

I’m just realized that this would not be allowed as I’ll have to include most of the theme sources as slightly modified overrides. I don’t really know, what to do with this. Personally I’d just used slightly modified default theme but I don’t know, how acceptable this would be in this project.

Let's keep modifying theme as it's done now. I've whipped a CI pipeline which will compile appstack theme and send PR's into frontend repo. So we can progress on this

@mrskycriper
Copy link
Contributor Author

mrskycriper commented Feb 4, 2024

Let's keep modifying theme as it's done now. I've whipped a CI pipeline which will compile appstack theme and send PR's into frontend repo. So we can progress on this

@nurikk I've allready spent a lot of hours on this without good progress. Fortunately, Appstack support said that in 2-3 weeks they will release an update with the proper dynamic theme support. In the mean time, I'll just continue working on other things.

PS please look at this issue #1897, it's still present.

@mrskycriper
Copy link
Contributor Author

And after all those fixes it worked, almost. It totally breaks dropdowns placement on the screen.

@nurikk I've found the culprit, custom animations in dropdowns under popper control are breaking it. Removed them from my test css and now it works fine.

Снимок экрана 2024-02-04 в 17 50 36

I've manually assembled unoptimized version of appstack css with working themes by extracting root variables and then packing a copy of each theme part in it's designated selector.

:root,
[data-bs-theme=light] {
    // variables
}

[data-bs-theme=dark] {
    // variables
}

[data-bs-theme=light] {
    // light.css from appstack /dist
}

[data-bs-theme=dark] {
    // dark.css from appstack /dist
}

So now I at least something to test the UI on.

@mrskycriper
Copy link
Contributor Author

mrskycriper commented Feb 11, 2024

@nurikk while we wait for the appstack release I took a look at translation keys and found a lot of missing keys that do not show in POEditor and thus can't be translated. How can I add new translation keys in the system?

@nurikk
Copy link
Owner

nurikk commented Feb 11, 2024

@nurikk while we wait for the appstack release I took a look at translation keys and found a lot of missing keys that do not show in POEditor and thus can't be translated. How can I add new translation keys in the system?

New keys are introduced to en.json, and later in semi automatic way to poedito

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

Successfully merging this pull request may close these issues.

None yet

2 participants