-
-
Notifications
You must be signed in to change notification settings - Fork 243
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
Home page: Add options to set simple navbar/standard background the default on mobile/desktop #2800
Conversation
#2382 Bundle Size — 10.84MiB (+0.03%).b8ed0c4(current) vs 0396d31 main#2380(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
Current #2382 |
Baseline #2380 |
|
---|---|---|
Initial JS | 1.9MiB (+0.06% ) |
1.89MiB |
Initial CSS | 577.21KiB |
577.21KiB |
Cache Invalidation | 22.77% |
24.09% |
Chunks | 226 |
226 |
Assets | 249 |
249 |
Modules | 2920 (+0.03% ) |
2919 |
Duplicate Modules | 149 |
149 |
Duplicate Code | 1.8% |
1.8% |
Packages | 96 |
96 |
Duplicate Packages | 2 |
2 |
Bundle size by type 2 changes
2 regressions
Current #2382 |
Baseline #2380 |
|
---|---|---|
JS | 9.05MiB (+0.03% ) |
9.05MiB |
CSS | 864.03KiB (~+0.01% ) |
863.98KiB |
Fonts | 526.1KiB |
526.1KiB |
Media | 295.6KiB |
295.6KiB |
IMG | 140.74KiB |
140.74KiB |
HTML | 1.38KiB |
1.38KiB |
Other | 871B |
871B |
Bundle analysis report Branch florian-h05:set-home-navbar Project dashboard
Generated by RelativeCI Documentation Report issue
0c474eb
to
b270001
Compare
@@ -7,7 +7,10 @@ | |||
"about.darkMode.dark": "Dark", | |||
"about.navigationBarsStyle": "Navigation bars style", | |||
"about.miscellaneous": "Miscellaneous", | |||
"about.miscellaneous.home.navbar": "Simple navigation bar on home page", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghys Do you know how I can remove the existing translations for this setting?
I need to get rid of them because there is a new text. Does CrowdIn do that automatically or do I have to remove this string from CrowdIn and from the i18n assets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea but I can check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! You can merge as soon as you want to - I will be away from keyboard next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems CrowdIn has properly recognized the changes 🚀
@ghys Please have a look at this PR. |
This adds a two new boolean config parameter to the home page settings, which allow to set the simple navbar as default on mobile and desktop devices. The navbar style setting in the about page has been changed from a toggle to a segmented button with the options default, simple, large. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
b270001
to
fbc5cbb
Compare
@ghys WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for stalling again 😞 and thanks for all the work.
I think it's a great idea to use the home page component config to store default values for all devices (optionally) and still have local configuration for individual devices to override them or not.
In fact it could be applied to the other option pertaining to the home page - the choice between a white or light gray background color while in 'light' mode.
(when you have a simple navbar in light mode to match the non-home pages I think you also want your home page background color to match the other non-home pages, i.e. light gray by default.)
And I'm just spitballing here but it could be a principle applied to even more Main UI-related options not pertaining to the home page (then we'll have to choose where to store them because storing "page transitions or not" in the home page component isn't right):
I mean:
"local settings and defaults set for you, unless there's global settings that are set, which will set the new default, but you can still override them locally"
As for this PR, well I don't like the segmented buttons too much - in general, not in this particular case (in translations you might have long strings and it wouldn't quite work anymore). But I don't have better proposals so it's all good to me 👍
…le or desktop Signed-off-by: Florian Hotze <dev@florianhotze.com>
Good point, I have added that in my last commit.
In that case we could (mis)use the uicomponents endpoint to use a UI component to store that config ... I agree with the segmented buttons, it is always a bit risky that translations get too long, but you said it, that's the best option here. |
This adds new boolean config parameters to the home page settings that allow setting the default navbar style and standard background for mobile and desktop devices.
The device-specific settings allow to override that default by using a segmented button with three options: The admin's default, option A and option B.
The segmented buttons are fully localized.