Skip to content

Added scrollable style focused to be displayed when mouse is over the scrollable area#1669

Merged
hecrj merged 8 commits into
iced-rs:masterfrom
GyulyVGC:master
Mar 27, 2023
Merged

Added scrollable style focused to be displayed when mouse is over the scrollable area#1669
hecrj merged 8 commits into
iced-rs:masterfrom
GyulyVGC:master

Conversation

@GyulyVGC

Copy link
Copy Markdown
Contributor

Hi! It's my first PR here for iced!

I added a style focused thought to be displayed when the scrollable area is hovered (not the scrollbar itself).

In my opinion, this is a more flexible solution to #1573.
In this way, a user can decide to hide the scroller when the area is not hovered or simply to have a different style from the active case, which would now be used when the scrollable area is not hovered.
If focused is not implemented, active will be used anyway.

If you don't like the name "focused", suggestions are welcomed!

Untitled.mp4

@hecrj hecrj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the name should be hovered instead of focused, like in a button::StyleSheet.

@hecrj hecrj added feature New feature or request widget styling labels Jan 24, 2023
@hecrj hecrj added this to the 0.8.0 milestone Jan 24, 2023
@GyulyVGC

Copy link
Copy Markdown
Contributor Author

I think the name should be hovered instead of focused, like in a button::StyleSheet.

The point is that hovered is already used to refer to the case when just the Scrollbar is hovered (instead of the whole scrollable area).

@GyulyVGC GyulyVGC requested a review from hecrj January 28, 2023 11:39
@GyulyVGC GyulyVGC marked this pull request as draft February 2, 2023 13:58
@GyulyVGC GyulyVGC marked this pull request as ready for review February 2, 2023 15:53
@hecrj hecrj modified the milestones: 0.8.0, 0.9.0 Feb 18, 2023
@GyulyVGC

Copy link
Copy Markdown
Contributor Author

@hecrj may I ask you what can I do to make this PR ready to be merged?

@hecrj

hecrj commented Mar 14, 2023

Copy link
Copy Markdown
Member

@GyulyVGC We need a better name here. focused is misleading.

@GyulyVGC

Copy link
Copy Markdown
Contributor Author

You are free to suggest a name of your preference.

Hovered would be awesome, but the problem is that it's already used.

@hecrj

hecrj commented Mar 14, 2023

Copy link
Copy Markdown
Member

I think we could just add a boolean over_scrollbar argument to the hovered method.

@GyulyVGC

GyulyVGC commented Mar 14, 2023

Copy link
Copy Markdown
Contributor Author

I think we could just add a boolean over_scrollbar argument to the hovered method.

Wouldn't this method also require to pass two distinct style arguments?

As an alternative, what about naming it focused and renaming the current focused to focused_scrollbar?

@hecrj

hecrj commented Mar 15, 2023

Copy link
Copy Markdown
Member

Wouldn't this method also require to pass two distinct style arguments?

No. The Style is always the same for both states of the scrollable.

@GyulyVGC

Copy link
Copy Markdown
Contributor Author

I think the name should be hovered instead of focused, like in a button::StyleSheet.

Following your suggestion, I renamed it to hovered.
The style that was previously hovered has been renamed to hovered_scrollbar.

@hecrj hecrj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

I just added the is_mouse_over_scrollbar flag as I suggested in the end. No need for additional methods.

@hecrj hecrj enabled auto-merge March 27, 2023 14:07
@hecrj hecrj merged commit 4e409bb into iced-rs:master Mar 27, 2023
@GyulyVGC

Copy link
Copy Markdown
Contributor Author

Amazing! Thanks for the corrections!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request styling widget

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants