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

Extend the touch zone for up button in touch menu. #5175

Merged
merged 1 commit into from Aug 3, 2019

Conversation

@robert00s
Copy link
Contributor

commented Aug 3, 2019

See: #5155
Close: #5155
I chose option 1 (red rectangle is touch zone).

Before:
obraz

After:
obraz

@robert00s robert00s added the UX label Aug 3, 2019

@Frenzie Frenzie added this to the 2019.08 milestone Aug 3, 2019

local up_button = IconButton:new{
icon_file = "resources/icons/appbar.chevron.up.png",
show_parent = self.show_parent,
padding_left = footer_width*0.33*0.1,

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 3, 2019

Member

Wouldn't it make more sense to base it on the IconButton itself or is that not known yet?

This comment has been minimized.

Copy link
@robert00s

robert00s Aug 3, 2019

Author Contributor

Size of Icon Button doesn't know yet. Beside of that better to set up left and right padding to 3,3% of footer size.

@Frenzie Frenzie merged commit 1743c0c into koreader:master Aug 3, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

Mhhh, my obsession with balance makes me not so pleased with that one :)

Previously, the left of the arrow button was aligned with the left edge of the line separator.
Now, on the screenshot, it looks aligned with the left edge of the menu items's text, which could be a valid decision (but on the right side of the footer, the 0% battery level is aligned with the right edge of the line separator...).
But if it looks aligned, it seems it is by chance :) and it may not with various screen ratio, font size, or display DPI, as the left padding of the text items is the actual size of a checkmark (whether it's invisible or not) - and not dependant of the self.padding used to pad the left of the button.

Personally, I would have prefered to just put no padding_left on the button, and double the padding_right, to get the same tapable width.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

I like padding left as the checkmark width. :)

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

I kinda like it too ;).

And I feel like, given the very shape of the icon, it might not suffer too much from slight horizontal misalignment with the text's edge.

(As I imagine accurately pinpointing the exact right amount of padding needed to align to the text might be slightly tricky?).

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Trickier than worth the effort I'd say but I doubt a workaround would take too long. Simplest/worst case you duplicate the CheckMark + whatever padding logic (should be a Size.padding of some sort), slightly trickier you'd export it somehow.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

OK, we'll see tomorrow on our devices :)

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

IIRC, the checkmark is tricky in that the width is mainly calculated by actually rendering a checkmark glyph and then getting it's bbox width...

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Slightly wasteful; not sure I'd call it tricky. :-P And by export I mean saving that width in an accessible property.

@robert00s robert00s deleted the robert00s:touch_menu_up branch Aug 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.