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

Option to show horizontal line separator in footer #5309

Merged
merged 10 commits into from Sep 5, 2019

Conversation

@robert00s
Copy link
Contributor

commented Sep 3, 2019

Hidden line separator (default)

New option Show footer separator

@Frenzie

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Do you intend to offer more options? Because just on/off is what the checkmark is for. ;-)

@Frenzie Frenzie added this to the 2019.09 milestone Sep 3, 2019

@Frenzie Frenzie added the UX label Sep 3, 2019

},
},
{
text = _("Items separator"),

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 3, 2019

Member
Suggested change
text = _("Items separator"),
text = _("Item separator"),
@robert00s

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

Do you intend to offer more options? Because just on/off is what the checkmark is for. ;-)

Of course :)

@Frenzie
Frenzie approved these changes Sep 3, 2019
Copy link
Member

left a comment

lgtm, also pinging @poire-z @NiLuJe

separator_line,
vertical_span,
self.footer_container,
}

This comment has been minimized.

Copy link
@poire-z

poire-z Sep 3, 2019

Contributor

I'd rather not see separator_line and vertical_span when not self.settings.bottom_horizontal_separator , instead of giving them a height of 0 (because that's not pretty :) and may be one day, they won't support a height of 0 :)
So, rather:

if self.settings.bottom_horizontal_separator then
  local separator_line = ...
  local vertical_span = ...
  self.vertical_frame = VerticalGroup:new{
        separator_line,
        vertical_span,
        self.footer_container,
    }
else
  self.vertical_frame = VerticalGroup:new{
        self.footer_container,
    }
end

It looks like we always access things by name, nowhere with indices like self.vertical_frame[3], so that should work.
(Even just self.vertical_frame = self.footer_container might work, but may be less risky keeping the same hierarchy.)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 3, 2019

Member

And even if we did use self.vertical_frame[3] that would generally make it a piece of code that should be changed to a named reference for clarity and maintainability instead.

Frenzie and others added 7 commits Sep 3, 2019
[Android] Don't ship Noto (#5310)
Follow-up to <#5252>. This greatly reduces the Android package size.

See discussion in <#5264 (comment)>.
Frontlight - Add checkbox use system settings (#5307)
See: #5205 (comment)

Devices with `hasLightLevelFallback = true` (for now Android) has extra checkbutton `Use system settings`. Default unchecked.
@poire-z

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

All fine to me with the last commit.

I get all the previous commits (cervantes, frontlight, noto) are just some other already commited commits that were brought in by some git update stuff, and they'll be no-op when merged.

But now, with this PR, as a reviewer, how can I be sure of that? And that there's not a new thing introduced in the other related files, like font.lua? (I trust @robert00s, it's just an academic question about using git/github and if it can help with that).

(I usually do followup work like in #4582 (comment) to avoid bringing alien commits in the PR.)

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

@Frenzie

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

It's a bit of a difficult question. On the one hand merging master makes it so that the branch always remains usable for people who are trying it out. They can just fetch/pull from the branch without issue. On the other hand git pull -r upstream master is much cleaner, but force pushing breaks things for people trying your branch and potentially kills some commit history relevant to the PR review process too.

For a perspective I mostly disagree with, see here. I prefer that the Git repo make sense without GH, which means a mixture between rebasing and squashing.

@robert00s

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

I should read more about git :)
Should I do something additional in this PR?

@Frenzie Frenzie merged commit 2d95a09 into koreader:master Sep 5, 2019

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details
@Frenzie

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

@robert00s It only just occurred to me now, but maybe status bar separator would be better?

@robert00s

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

Status bar as separator will be next step :)
And I saw yesterday in new Kobo Libra, status bar at the very bottom of edge:

obraz
I like it.

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

The FW update download progress bar is similar (although at the top of the screen, and possibly thicker) ;).

(That was a fun useless thought I had when I noticed the "new" progress bar ;)).

@Frenzie

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

What's the stuff at the top & bottom? Top total pages, bottom chapter pages?

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

I'd assume book on top, chapter at the bottom, yeah. I think David detailed it properly in the Libra thread on MR.

@robert00s robert00s deleted the robert00s:line_separator branch Sep 21, 2019

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