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

Update lists, visually merge album header #402

Merged
merged 18 commits into from Jan 12, 2022
Merged

Conversation

jannuary
Copy link
Contributor

  • Don't use .card, use custom style for playlists instead
    One thing about the previous MR is that .card applies to the entire ListView, including stuff like rounded corners. Herein, I try to recreate the .boxed-list style, applying the style to individual rows.
  • Use AdwClampScrollable
    In practice, this means that the scrollbar is where it's supposed to be while technically being under ListView ∈ ScrolledWindow rule.
  • Visually merge album header & headerbar
    Make header revealing/collapsing feel more intentional.

image

Resolves #379

@xou816
Copy link
Owner

xou816 commented Dec 13, 2021

I think I preferred the color scheme in your original mockup (darker header, lighter list)

TIL about AdwScrollableClamp, nice! is the ScrollableWindow needed at all in that case?

@xou816 xou816 closed this Dec 13, 2021
@xou816 xou816 reopened this Dec 13, 2021
@jannuary
Copy link
Contributor Author

I think I preferred the color scheme in your original mockup (darker header, lighter list)

Does view_bg look fine?
image
Thing is, we can't exactly have a 'darker' color without having a totally separate light and dark css.

TIL about AdwScrollableClamp, nice! is the ScrollableWindow needed at all in that case?

Clamp doesn't implement any of the scrolling business afaik, it's just a helper widget of sorts.

@jannuary
Copy link
Contributor Author

Thing is, we can't exactly have a 'darker' color without having a totally separate light and dark css.

I guess we could use AdwApplication - it provides separate css for free, more or less.

@jannuary
Copy link
Contributor Author

Thing is, we can't exactly have a 'darker' color without having a totally separate light and dark css.

I guess we could use AdwApplication - it provides separate css for free, more or less.

Also, there has to be some sorta transition between header header and headerbar header.

@xou816
Copy link
Owner

xou816 commented Dec 22, 2021

Also, there has to be some sorta transition between header header and headerbar header.

what kind of transition are you thinking of, is it about having the title appear in the headerbar?

I guess we could use AdwApplication - it provides separate css for free, more or less.

I'll have a look, not sure what this allows! don't worry too much about the bg color otherwise, if you like it that way I think I can live with it :D

@jannuary
Copy link
Contributor Author

jannuary commented Dec 27, 2021

One more thing - I want to make the entire header area draggable, but that conflicts with the swipe tracker - clicking on the header area immideately makes it collapse. Anything that could be done about that?

Is it fine to have the swipe gesture be touch-only?

@xou816
Copy link
Owner

xou816 commented Dec 28, 2021

Clicking should not make it collapse, it should not identify a simple click as a swipe so there could be a small issue here, but yes, absolutely we could make it touch only! I am not sure how that would work with a GtkWindowHandle which I assume you´d want here?

@xou816
Copy link
Owner

xou816 commented Dec 28, 2021

Actually I'm saying we can make it touch only but I can't remember how at the moment... I think there was a flag for that in gtk3...

@jannuary
Copy link
Contributor Author

Clicking should not make it collapse, it should not identify a simple click as a swipe

Yeah, pardon, it doesn't indeed - I meant it collapses on drag.

@xou816
Copy link
Owner

xou816 commented Dec 28, 2021

Oh yeah okay checked out your branch (looking good!!) and I see what you mean :/

not sure how to work around that! Except to make it touch only indeed

@xou816
Copy link
Owner

xou816 commented Dec 28, 2021

Well set_touch_only seems to do the trick :D

@xou816
Copy link
Owner

xou816 commented Jan 2, 2022

Well set_touch_only seems to do the trick :D

(diff for reference)

--- a/src/app/components/details/details.rs
+++ b/src/app/components/details/details.rs
@@ -97,9 +97,11 @@ impl AlbumDetailsWidget {
         );
 
         let swipe_controller = gtk::GestureSwipe::new();
+        swipe_controller.set_touch_only(true);
         swipe_controller.set_propagation_phase(gtk::PropagationPhase::Capture);
         swipe_controller.connect_swipe(clone!(@weak self as _self => move |_, _, dy| {
-            _self.set_header_visible(dy >= 0f64);
+            _self.set_header_visible(dy > -0.1);
         }));
 
         self.widget()

I see you're still making changes to the style, just give me a heads up when you want to merge things:)

@jannuary jannuary marked this pull request as draft January 3, 2022 11:24
@jannuary
Copy link
Contributor Author

jannuary commented Jan 3, 2022

Have unpushed commits and several other nitpicks and also not at home, so making a draft

@xou816
Copy link
Owner

xou816 commented Jan 3, 2022

Perfect 👍

@jannuary jannuary marked this pull request as ready for review January 5, 2022 04:58
@jannuary jannuary marked this pull request as draft January 5, 2022 05:08
@jannuary jannuary marked this pull request as ready for review January 5, 2022 05:23
@jannuary
Copy link
Contributor Author

jannuary commented Jan 5, 2022

Alright, outside of the headerbar transition, I'm happy with this now.

@xou816
Copy link
Owner

xou816 commented Jan 5, 2022

I have one or two things I need to do with headerbars anyway, maybe I could help! what did you have in mind again? Have the album title appear in the header when scrolling is that right?

@jannuary
Copy link
Contributor Author

jannuary commented Jan 5, 2022

Removing .flat and showing the title when the header is hidden, yeah

@xou816
Copy link
Owner

xou816 commented Jan 5, 2022

https://github.com/xou816/spot/compare/listing-headerbar here is an update for that! I should not have made this Screen-wrapper thing but long story short, should be easier to use our selection-enabled headerbars now

if that looks okay to you I'll push to your branch!

image

image

not so sure how to make the css transition good but it's a start

@xou816
Copy link
Owner

xou816 commented Jan 11, 2022

Two small things:

image

I think we could use some spacing between the headerbar and the content

image

Maybe there should be a border there as well?

@@ -87,7 +87,7 @@ impl AlbumDetailsWidget {
let is_up_to_date = widget.header_revealer.reveals_child() == visible;
if !is_up_to_date {
widget.header_revealer.set_reveal_child(visible);
widget.headerbar.set_title_visible(!visible);
widget.headerbar.set_title_visible(true);
Copy link
Owner

Choose a reason for hiding this comment

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

think the line can be removed altogether as well as the property in the ui file if we're using opacity now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is a dirty hack - title was invisible if not explicitly set visible somehow.

@jannuary
Copy link
Contributor Author

Maybe there should be a border there as well?

Not sure why it doesn't - I removed the border from the main headerbar as to not interfere with the clamp's but it shouldn't affect selection mode?

@jannuary
Copy link
Contributor Author

Yeah, I'm not touching its box-shadow at all, it just doesn't get rendered for some reason

@xou816
Copy link
Owner

xou816 commented Jan 12, 2022

ah well, we'll figure this out later! thank you so much, those screens you've been working on look so much better than the others!!

think it might be time to release a new version on master too, if its compatible with the currently released runtime!

@xou816 xou816 merged commit 9d1b766 into xou816:development Jan 12, 2022
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.

Use AdwClampScrollable
2 participants