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

[sailfish-office] Increase the size of the page index in toolbar. Contributes to TJC#212568 #139

Merged
merged 2 commits into from Sep 10, 2019

Conversation

dcaliste
Copy link
Contributor

@dcaliste dcaliste commented Sep 2, 2019

Following discussion in TJC thread, I propose to expand a bit the size of the page index in PDF toolbar. For documents with many pages (like more than 100), the current label is shrinking to fit into a itemSizeSmall width.

I propose to expand this to itemSizeMedium, and to avoid to have wider space for documents with few pages (< 10), I'm changing a bit the width calculation logic to (label.implicitWidth + padding) limited by itemSizeMedium.

@dcaliste
Copy link
Contributor Author

dcaliste commented Sep 2, 2019

@pvuorela or @jpetrell if you have time to give feedback. I'm keeping the limitation of the label width, as done before, even if the toolbar is now more consise in width with icons more packed.

Besides, I've checked that when the back button is visible, the new index page width is not putting things out of the screen. Doing this, I've noticed that the back button was not properly vertically aligned. I've pushed another commit on top.

@dcaliste
Copy link
Contributor Author

dcaliste commented Sep 9, 2019

@pvuorela did you have time to look at this one? Discard the question if you're busy!

@pvuorela
Copy link
Contributor

Looks a lot better now, thanks. LGTM. The page number label could increase width even more if needed, but that could then require more advanced layouting, should be good enough for most cases already.

@pvuorela pvuorela merged commit e715d0a into sailfishos:master Sep 10, 2019
@dcaliste dcaliste deleted the page branch September 10, 2019 11:33
@dcaliste
Copy link
Contributor Author

Great, thank you @pvuorela

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.

None yet

2 participants