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

Attempt to fix vertical padding on pixel coordinate div in imviz #4

Merged
merged 2 commits into from
Dec 1, 2021

Conversation

kecnry
Copy link

@kecnry kecnry commented Nov 30, 2021

Description

The font that is rendered by notebook vs lab seem to be slightly different, so centering with a fixed padding is inconsistent.
This pull request attempts to give more consistent padding to the pixel display in imviz by vertically centering the div and providing a fixed line height.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a change log needed? If yes, is it added to CHANGES.rst?
  • Is a milestone set? Milestone is only currently required for PRs related to Imviz MVP.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

* attempt to fix padding inconsistencies between browsers and jupyter notebook vs lab
@kecnry kecnry requested a review from pllim as a code owner November 30, 2021 14:34
Copy link
Owner

@pllim pllim left a comment

Choose a reason for hiding this comment

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

The padding is fixed but "Pixel" row now jumps to the center when WCS is not present.

What do you think, @Jenneh ?

Without this patch

Screenshot 2021-11-24 012714

Screenshot 2021-11-24 012038

Screenshot 2021-11-24 011948

With this patch

Screenshot 2021-11-30 123724

Screenshot 2021-11-30 123857

Screenshot 2021-11-30 123934

@kecnry
Copy link
Author

kecnry commented Nov 30, 2021

Yes, that's true, it will always vertically center the contents. If we don't want that, we can add empty placeholders with a fixed height for the other "rows" so that the pixel row is always near the top.

@pllim
Copy link
Owner

pllim commented Nov 30, 2021

I don't have a preference but maybe @PatrickOgle or @Jenneh does.

@rosteen
Copy link

rosteen commented Dec 1, 2021

See https://vuetifyjs.com/en/components/grids/ for vuetify alignment stuff, specifically https://vuetifyjs.com/en/components/grids/#align.

@rosteen
Copy link

rosteen commented Dec 1, 2021

You may need to put a <v-container> inside the div to take advantage of the vuetify features.

@pllim
Copy link
Owner

pllim commented Dec 1, 2021

I can't get VueJS to work. I tried adding a <v-container> <v-row align="start"> <v-col> layer outside of <div> and I get something horrible. I found a non-VueJS workaround though.

Screenshot 2021-12-01 144200

@pllim
Copy link
Owner

pllim commented Dec 1, 2021

Thanks, everyone!

@pllim pllim merged commit 4120379 into pllim:show-me-the-degrees Dec 1, 2021
@kecnry kecnry deleted the pixel-div-vert-centered branch December 2, 2021 13:48
@Jenneh
Copy link

Jenneh commented Dec 2, 2021

It is nice that it always vertically centers. Padding looks better. It is still a little uncomfortably close to the edges but I think this is good enough for now.

pllim added a commit that referenced this pull request Nov 3, 2023
#4)

* Lim edits

* Improve verbiage in docstring

Co-authored-by: Brett M. Morris <[email protected]>

* Address review comments from bmorris3

* Add multi WCS-only test back

* Update concept notebook

* corrections to the pr to the pr to the pr (#10)

* corrections to the pr to the pr to the pr

* using app-level attr

* Update jdaviz/configs/imviz/plugins/parsers.py

Co-authored-by: P. L. Lim <[email protected]>

* Update jdaviz/configs/imviz/plugins/parsers.py

Co-authored-by: P. L. Lim <[email protected]>

---------

Co-authored-by: P. L. Lim <[email protected]>

* Follow up to PR to my PR to your PR

* WCS-only must be loaded
into viewer but not visible.

Fix concept notebook

---------

Co-authored-by: Brett M. Morris <[email protected]>
pllim added a commit that referenced this pull request Nov 15, 2023
#4)

* Lim edits

* Improve verbiage in docstring

Co-authored-by: Brett M. Morris <[email protected]>

* Address review comments from bmorris3

* Add multi WCS-only test back

* Update concept notebook

* corrections to the pr to the pr to the pr (#10)

* corrections to the pr to the pr to the pr

* using app-level attr

* Update jdaviz/configs/imviz/plugins/parsers.py

Co-authored-by: P. L. Lim <[email protected]>

* Update jdaviz/configs/imviz/plugins/parsers.py

Co-authored-by: P. L. Lim <[email protected]>

---------

Co-authored-by: P. L. Lim <[email protected]>

* Follow up to PR to my PR to your PR

* WCS-only must be loaded
into viewer but not visible.

Fix concept notebook

---------

Co-authored-by: Brett M. Morris <[email protected]>
pllim added a commit that referenced this pull request Dec 8, 2023
#4)

* Lim edits

* Improve verbiage in docstring

Co-authored-by: Brett M. Morris <[email protected]>

* Address review comments from bmorris3

* Add multi WCS-only test back

* Update concept notebook

* corrections to the pr to the pr to the pr (#10)

* corrections to the pr to the pr to the pr

* using app-level attr

* Update jdaviz/configs/imviz/plugins/parsers.py

Co-authored-by: P. L. Lim <[email protected]>

* Update jdaviz/configs/imviz/plugins/parsers.py

Co-authored-by: P. L. Lim <[email protected]>

---------

Co-authored-by: P. L. Lim <[email protected]>

* Follow up to PR to my PR to your PR

* WCS-only must be loaded
into viewer but not visible.

Fix concept notebook

---------

Co-authored-by: Brett M. Morris <[email protected]>
pllim added a commit that referenced this pull request Dec 8, 2023
#4)

* Lim edits

* Improve verbiage in docstring

Co-authored-by: Brett M. Morris <[email protected]>

* Address review comments from bmorris3

* Add multi WCS-only test back

* Update concept notebook

* corrections to the pr to the pr to the pr (#10)

* corrections to the pr to the pr to the pr

* using app-level attr

* Update jdaviz/configs/imviz/plugins/parsers.py

Co-authored-by: P. L. Lim <[email protected]>

* Update jdaviz/configs/imviz/plugins/parsers.py

Co-authored-by: P. L. Lim <[email protected]>

---------

Co-authored-by: P. L. Lim <[email protected]>

* Follow up to PR to my PR to your PR

* WCS-only must be loaded
into viewer but not visible.

Fix concept notebook

---------

Co-authored-by: Brett M. Morris <[email protected]>
pllim added a commit that referenced this pull request Jan 23, 2024
#4)

* Lim edits

* Improve verbiage in docstring

Co-authored-by: Brett M. Morris <[email protected]>

* Address review comments from bmorris3

* Add multi WCS-only test back

* Update concept notebook

* corrections to the pr to the pr to the pr (#10)

* corrections to the pr to the pr to the pr

* using app-level attr

* Update jdaviz/configs/imviz/plugins/parsers.py

Co-authored-by: P. L. Lim <[email protected]>

* Update jdaviz/configs/imviz/plugins/parsers.py

Co-authored-by: P. L. Lim <[email protected]>

---------

Co-authored-by: P. L. Lim <[email protected]>

* Follow up to PR to my PR to your PR

* WCS-only must be loaded
into viewer but not visible.

Fix concept notebook

---------

Co-authored-by: Brett M. Morris <[email protected]>
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.

4 participants