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

chore: Bump dependencies #1818

Merged
merged 5 commits into from
Sep 24, 2024
Merged

chore: Bump dependencies #1818

merged 5 commits into from
Sep 24, 2024

Conversation

spydon
Copy link
Collaborator

@spydon spydon commented Sep 19, 2024

No description provided.

@spydon spydon requested a review from d-loose September 19, 2024 13:46
@d-loose
Copy link
Member

d-loose commented Sep 19, 2024

Thanks for working through all those dependencies!

Some things I notices while testing a snap build of this:

  • accent colors work correctly on both 24.04 and 24.10 - great work @Feichtmeier!
  • the background color and border style of the cards has changed again
light dark
Screenshot from 2024-09-19 16-29-23 Screenshot from 2024-09-19 16-29-15

I feel like it looks a bit off in the light theme

@Feichtmeier
Copy link
Member

Thanks for working through all those dependencies!

Some things I notices while testing a snap build of this:

  • accent colors work correctly on both 24.04 and 24.10 - great work @Feichtmeier!
  • the background color and border style of the cards has changed again

light dark
Screenshot from 2024-09-19 16-29-23 Screenshot from 2024-09-19 16-29-15
I feel like it looks a bit off in the light theme

np, yw!

indeed too strong!
https://github.com/ubuntu/yaru.dart/blob/d0cc369a08bb5a7273b44cbdd3f059dc208ddbe2/lib/src/widgets/yaru_banner.dart#L94
don't know how I came to this color but it should just be cardcolor, and we need to make sure that cardcolor has an appr. contrast against surface
however for app center you prbly dont want this strong contrast (I'm not saying this is perfect I need to look into it again, long time no use of this widget) so there are two color properties, I think you can just take scaffold background color or surface for both, I really need to look into this widget again, I think most things are not really needed (anymore)

@d-loose
Copy link
Member

d-loose commented Sep 19, 2024

@spydon: looks like you left a TODO note for this :)

// TODO: Remove color once we have upgraded to a yaru version > 4.1.0

@spydon spydon force-pushed the chore/bump-dependencies branch from 55a0de5 to a032d78 Compare September 19, 2024 15:22
@d-loose
Copy link
Member

d-loose commented Sep 19, 2024

I'm just noticing that yaru doesn't catch any gsettings exceptions in case accent colors aren't supported

flutter: ERROR main: Error propagated to top-level
	GSettingsUnknownKeyException: Key accent-color not in GSettings schema org.gnome.desktop.interface
#0      GSettings._getSchemaEntry (package:gsettings/src/gsettings.dart:199:7)
#1      GSettings.get (package:gsettings/src/gsettings.dart:120:23)
<asynchronous suspension>
#2      GnomeSettings._updateValue.<anonymous closure> (package:yaru/src/theme_widgets/settings_service.dart:59:31)
<asynchronous suspension>

@Feichtmeier
Copy link
Member

I don't know if this is enough though
When I look into the example I think it's too strong there too
If you want to wait I can push 🫸 a change/release later otherwise I would suggest you just overwrite the colour with whatever you want

@d-loose
Copy link
Member

d-loose commented Sep 19, 2024

I'd definitely prefer making such changes in yaru.dart so that we can use it everywhere and get consistent themeing without needing to maintain any application-local customizations.

@spydon
Copy link
Collaborator Author

spydon commented Sep 19, 2024

I think the color that we used to override with before was good? So YaruBanner now uses cardColor by default, but cardColor was changed, was there any reason for that change @Feichtmeier, if you remember? :)

Screenshot from 2024-09-19 17-19-43

@Feichtmeier
Copy link
Member

Feichtmeier commented Sep 19, 2024

I think the color that we used to override with before was good? So YaruBanner now uses cardColor by default, but cardColor was changed, was there any reason for that change @Feichtmeier, if you remember? :)

Screenshot from 2024-09-19 17-19-43

sorry I dont remember 👴
just gimme a hex and I will make it work now (for all yaru.dart)
edit: two hex

@spydon
Copy link
Collaborator Author

spydon commented Sep 19, 2024

@Feichtmeier aah, it's defined in relation to the surface color:

Color _cardColor(ColorScheme colorScheme) =>
    colorScheme.surface.scale(lightness: colorScheme.isLight ? -0.1 : 0.08);

And before it seems like cardColor was just the surface color directly, seems like it would make sense to have them different at least? Maybe we should pull in @anasereijo here.

@Feichtmeier
Copy link
Member

@Feichtmeier aah, it's defined in relation to the surface color:

Color _cardColor(ColorScheme colorScheme) =>
    colorScheme.surface.scale(lightness: colorScheme.isLight ? -0.1 : 0.08);

And before it seems like cardColor was just the surface color directly, seems like it would make sense to have them different at least? Maybe we should pull in @anasereijo here.

yes, the problem is that cardColor needs to have a contrast against the surface
maybe we just use a different color (the one you wish) for YaruBanner?

@Feichtmeier
Copy link
Member

is this good?

image
image

@Feichtmeier
Copy link
Member

or rather this?

image
image

@spydon
Copy link
Collaborator Author

spydon commented Sep 19, 2024

is this good?

image
image

I think @anasereijo said she'd have some time to check tomorrow, so tagging her again 😄

@Feichtmeier
Copy link
Member

Feichtmeier commented Sep 19, 2024

ok I push something that looks like what you have in app center now if this is okay

light: scaffold color
image

dark: slight contrast
image

@anasereijo
Copy link
Collaborator

ok I push something that looks like what you have in app center now if this is okay

light: scaffold color image

dark: slight contrast image

Thanks @Feichtmeier this is looking much better IMO. I wasn't sure about the grey background on app cards, so thanks for making it possible to have the white. I quite like the subtle contrast in the dark theme too 👍 Thanks!

@spydon
Copy link
Collaborator Author

spydon commented Sep 20, 2024

@anasereijo this is what it looks like with Yaru 5.2.1:
Screenshot from 2024-09-20 18-01-12
Screenshot from 2024-09-20 18-01-24

Copy link
Member

@d-loose d-loose left a comment

Choose a reason for hiding this comment

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

Thanks a lot - LGTM 👍

@d-loose d-loose merged commit bbd3b9f into main Sep 24, 2024
8 checks passed
@d-loose d-loose deleted the chore/bump-dependencies branch September 24, 2024 10:05
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.

4 participants