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

Variable viewer height #1435

Merged
merged 3 commits into from
Apr 14, 2020
Merged

Variable viewer height #1435

merged 3 commits into from
Apr 14, 2020

Conversation

jessicaschilling
Copy link
Contributor

closes #1434

Makes PDF viewer height take up full screen height
Unfortunately can't be based entirely on Tachyons references since fixed pixel heights for assorted header items; suggest eventual rework to fully integrate Tachyons throughout WebUI
@jessicaschilling
Copy link
Contributor Author

@hacdias and/or @alanshaw, can you please take a look? There may be a more elegant way of doing this via Tachyons only, but I wasn't finding it -- there are enough explicitly sized things in the header matter of the file viewer and in the app header at medium/below width that I couldn't find a way myself. Thanks!

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

I think it's okay for now. Thanks for this @jessicaschilling 😃

P.S.: I'm not aware of any way of doing this only with tachyons.

@jessicaschilling
Copy link
Contributor Author

@hacdias -- thanks for the review. I'm a bit antsy about releasing this on my own; any suggestions for someone who could walk through with me in your absence?

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

❤️ I tried this out and it works for me 👍 It's probably fine but the magic numbers make me a bit nervous and I think it could be improved by using flexbox instead (which may allow a tachyons only solution too).

Screenshot 2020-04-14 at 11 00 10

src/files/file-preview/FilePreview.css Outdated Show resolved Hide resolved
@jessicaschilling
Copy link
Contributor Author

Thanks @alanshaw -- added comments. It'd be great to get this out to people, but I've not released webui before -- do you have any time at all to advise on the process or could I bribe you to do so? Is this worthy of its own release or should it be batched with other small fixes?

@alanshaw
Copy link
Member

I've not done it before - did you see https://github.com/ipfs-shipyard/ipfs-webui#releasing-a-new-version-of-the-webui ?

I think #1432 needs to go in asap so might be worth getting both in before a new release.

cc @lidel

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

👌

@jessicaschilling jessicaschilling merged commit dd09f39 into master Apr 14, 2020
@lidel lidel deleted the bug-1434-viewerheight branch April 14, 2020 15:04
@lidel
Copy link
Member

lidel commented Apr 14, 2020

(We now wait for master to build and copy CID)

I'll test it and PR repos listed in #releasing-a-new-version-of-the-webui and cc @jessicaschilling as a passenger on the release train ;-)

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.

PDF viewer doesn't scale to page
4 participants