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

Change the default navbar style from orange to light for light theme #2288

Merged
merged 2 commits into from Feb 23, 2024

Conversation

florian-h05
Copy link
Contributor

@florian-h05 florian-h05 commented Jan 26, 2024

This is a proposal, please discuss it.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05 florian-h05 requested a review from a team as a code owner January 26, 2024 23:40
Copy link

relativeci bot commented Jan 26, 2024

Job #1644: Bundle Size — 11.01MiB (~-0.01%).

38f9e57(current) vs 843ee90 main#1643(baseline)

Warning

Bundle contains 19 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes Improvement 1 improvement
                 Current
Job #1644
     Baseline
Job #1643
Improvement  Initial JS 1.85MiB(~-0.01%) 1.85MiB
No change  Initial CSS 607.89KiB 607.89KiB
Change  Cache Invalidation 22.24% 17.59%
No change  Chunks 220 220
No change  Assets 242 242
No change  Modules 3088 3088
No change  Duplicate Modules 164 164
No change  Duplicate Code 1.77% 1.77%
No change  Packages 150 150
No change  Duplicate Packages 18 18
Bundle size by type  Change 1 change Improvement 1 improvement
                 Current
Job #1644
     Baseline
Job #1643
Improvement  JS 9.2MiB (~-0.01%) 9.2MiB
Not changed  CSS 889.44KiB 889.44KiB
Not changed  Fonts 526.1KiB 526.1KiB
Not changed  Media 295.6KiB 295.6KiB
Not changed  IMG 140.74KiB 140.74KiB
Not changed  HTML 1.24KiB 1.24KiB
Not changed  Other 871B 871B

View job #1644 reportView florian-h05:change-default-bars-... branch activityView project dashboard

@florian-h05
Copy link
Contributor Author

@florian-h05 florian-h05 added enhancement New feature or request main ui Main UI labels Jan 27, 2024
@ghys
Copy link
Member

ghys commented Jan 27, 2024

before you merge: why?

@florian-h05
Copy link
Contributor Author

I put this as a proposal to discuss it, so I'm happy that you ask.

Whilst the orange navbar shows openHAB's color, the orange can be quite "pushy", so @mstormi asked me to make the normal navbar the default.

WDYT?

@lolodomo
Copy link
Contributor

Can we see a screenshot?
My general feeling is that we should keep OH main color.

@mstormi
Copy link

mstormi commented Jan 27, 2024

You can already switch navbar to that mode (via "about" menu), it's just about making that the default.

@florian-h05 florian-h05 marked this pull request as draft January 28, 2024 21:38
@ghys
Copy link
Member

ghys commented Jan 29, 2024

I put this as a proposal to discuss it, so I'm happy that you ask.

Sorry I reacted urgently as I was unsure if you would merge this directly, and I'm admitting I didn't see https://community.openhab.org/t/permastore-ui-look-options/152052/7, so thanks for the link.

For full disclosure, I remember my initial UI project and it had the iOS and desktop themes set to light/dark and only the Android theme had the orange navbar (in light theme only), as Android apps tended to have such colored navbars at the time.
In fact it's still there: https://github.com/ghys/openhab-ui-f7/blob/47d5bf7a0c08defc902a92b079dbe93cf61a6e50/src/components/app.vue#L258
compared to now where the desktop theme gets the 'filled' bars unless in dark mode:

this.themeOptions.bars = localStorage.getItem('openhab.ui:theme.bars') || ((this.$theme.ios || this.$f7.darkTheme || this.themeOptions.dark === 'dark') ? 'light' : 'filled')

I seem to remember a conversation with @kaikreuzer (but I do not remember if it was on GitHub or on Slack) convincing me to put the filled bars on the desktop mode, as it was the 'brand color' - again, IIRC. I just didn't want those filled bars on iOS as I thought it was very un-iOSy to have filled bars on iPhones so I agreed.

So there you have it, I don't offer an opinion, just the context.

@florian-h05
Copy link
Contributor Author

Thanks for sharing the context.
I think that Main UI should look like the OS you are using it on, so if having these coloured bars is still common on Android, I’d keep them.

I haven’t used an Android phone for a while now, is that still the common Android design to have coloured bars?

@florian-h05 florian-h05 removed the enhancement New feature or request label Feb 15, 2024
@florian-h05
Copy link
Contributor Author

@openhab/android-maintainers Can you please tell me whether Android still often users coloured nav bars?

@mueller-ma
Copy link
Member

The Android app uses a light/dark top bar since the Material You redesign.

@florian-h05
Copy link
Contributor Author

Okay thanks.

WDYT matches better to Material You? The orange navbar or the light/dark navbar?

@maniac103
Copy link

WDYT matches better to Material You? The orange navbar or the light/dark navbar?

I'd say the light one (is this orange-on-white?), as the app itself doesn't have a colored top bar anymore and thus the phone status bar has a white background as well:

foo

Ideally we could pass an accent color into the UI as well, but I imagine that would be larger effort...

@florian-h05
Copy link
Contributor Author

Your screenshot shows filled (this is orange on white), which is the current default on Android.

So we should keep this default?

@maniac103
Copy link

So we should keep this default?

My screenshot was intended to show this default should not be kept ;-) With older versions of the app, the status bar background was orange as well, as was the whole app title bar. Nowadays, the app title bar is white(-ish, might change slightly with dynamic theme):

Screenshot_20240223-080954

... thus I think the default should be changed, since the orange bar in combination with white status bar looks a bit out of place now. (I wasn't aware of this being an option, or I would have raised an issue before)

@florian-h05
Copy link
Contributor Author

Thanks for the clarification, so we should use the light (non-orange) bar as default for Android.

I will update this PR to only keep the orange bars as default for the desktop theme.

@mstormi
Copy link

mstormi commented Feb 23, 2024

I will update this PR to only keep the orange bars as default for the desktop theme.

Why? The orange is pushy, no matter if Android or desktop.

@florian-h05
Copy link
Contributor Author

True, but I'm hesitant because it still is the "brand-color" and no one else has complained yet.
But I guess since on desktop the openHAB logo in the side-menu is likely to be visible, this is not a big problem, so let's change it.
And the openHAB color is at least the accent color in the UI.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor Author

Let me just switch the order of the navigation bar style pickers, so the new default is still on the left.

@florian-h05 florian-h05 marked this pull request as ready for review February 23, 2024 10:55
@florian-h05 florian-h05 changed the title Change the default navbar style from orange to light/dark Change the default navbar style from orange to light Feb 23, 2024
@florian-h05 florian-h05 changed the title Change the default navbar style from orange to light Change the default navbar style from orange to light for light theme Feb 23, 2024
@florian-h05 florian-h05 merged commit a1f5091 into openhab:main Feb 23, 2024
6 checks passed
@florian-h05 florian-h05 deleted the change-default-bars-style branch February 23, 2024 11:11
@florian-h05 florian-h05 added the enhancement New feature or request label Feb 23, 2024
@florian-h05 florian-h05 added this to the 4.2 milestone Feb 23, 2024
seime pushed a commit to seime/openhab-webui that referenced this pull request Mar 16, 2024
…penhab#2288)

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Arne Seime <arne.seime@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants