Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: make height controllable in other context #505

Merged
merged 3 commits into from
Apr 27, 2021

Conversation

mariobuikhuizen
Copy link
Collaborator

Using height: 100% is enough here to always take up all space of
the parent. The total height of the entire app can then be
specified elsewhere.

Part of #503

Using height: 100% is enough here to always take up all space of
the parent. The total height of the entire app can then be
specified elsewhere.

Part of spacetelescope#503
@pllim pllim added the bug Something isn't working label Apr 8, 2021
mariobuikhuizen added a commit to mariobuikhuizen/standalone_jdaviz that referenced this pull request Apr 8, 2021
mariobuikhuizen added a commit to mariobuikhuizen/standalone_jdaviz that referenced this pull request Apr 8, 2021
Copy link
Contributor

@javerbukh javerbukh left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

Doesn't break anything at least, but I didn't confirm that it helps in the MAST context. This is only part of solving that problem, right?

@javerbukh
Copy link
Contributor

@havok2063 Can you confirm that this works for you or at least helps with the issues you were seeing?

Copy link
Collaborator

@havok2063 havok2063 left a comment

Choose a reason for hiding this comment

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

So this seems reasonable to me, however, when I test this in our existing z.mast site, which does not contain any explicit height settings, I get the attached image, where it appears it has rendered with 0 height. Does this change need to be combined with changes from the on-going work in havok2063/standalone_jdaviz#1? (I did manage to get some height rendered using the standalone site digging through some of your changes).

In my head, I feel like the workflow should be something like:

  1. If a user embeds jdaviz as is with no CSS modifications, the height of jdaviz should be some default, probably the settings.context.notebook.max_height.
  2. The user can modify the height of jdaviz by applying a CSS modifcation to the designated application id or class name.
  3. Jdaviz should always attempt to fill the height 100% of the parent container

If it doesn't already, I feel like jdaviz should have a custom class that contains its entire CSS stylings that can be easily overridden. This makes sense to me especially if we consider jdaviz as a component in a "javascript" library once it gets embedded. If we expand this concept out to the most general case, every piece of jdaviz (e.g. icon, color, shape, size, etc) should be easily overridable by a user when they embed it. This is probably a large scope item but could be a noble goal.

Can you provide an example of how to flexibly change the height of jdaviz?

Screen Shot 2021-04-12 at 12 39 08 PM

@havok2063
Copy link
Collaborator

I'll also add that if this is one step in the process of making it easier for users to change the height, then I'm ok approving and merging this in. The notebooks still render.

@mariobuikhuizen
Copy link
Collaborator Author

The changes in havok2063/standalone_jdaviz#1 are indeed an example on what's required to make it work in the host site.

It's not possible to use the settings.context.notebook.max_height in the CSS part of the template, so that would still have to be set in a style attribute, which then can only be changed from CSS by using '!important' or by setting the style attribute of <jupyter-widget ..> element.

Also, point 1 and 3 are mututally exclusive.

To make things worse, some CSS is needed anyway to fix the height of the nested v-app jdaviz uses (which would other wise be the height of the view), see: https://github.com/havok2063/standalone_jdaviz/blob/6843a34d1207a5db33f2b2171a17a174ed91404c/example/static/css/jdaviz.css#L6

So the site has to contain the CSS of https://github.com/havok2063/standalone_jdaviz/blob/6843a34d1207a5db33f2b2171a17a174ed91404c/example/static/css/jdaviz.css anyway (in which I added the default height of 400px as an example, this could also be 100%) and the user must add the class jdaviz-panel on the <jupyter-widget> element that contains jdaviz.

Come to think of it, we can also set a class on the top jdaviz element, so setting the class jdaviz-panel isn't needed anymore.

@mariobuikhuizen mariobuikhuizen requested a review from ibusko as a code owner April 14, 2021 08:49
mariobuikhuizen added a commit to mariobuikhuizen/standalone_jdaviz that referenced this pull request Apr 14, 2021
Copy link
Contributor

@ibusko ibusko left a comment

Choose a reason for hiding this comment

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

In the notebook it runs fine, nothing seems to be broken. I can't tell anything about how this runs in the MAST context though, so I am approving with this caveat.

@havok2063
Copy link
Collaborator

Can we create a default jdaviz.css file that defines a new jdaviz-panel class, and contains all the defaults used by the application, e.g a default height? That way end users could easily customize settings inside jdaviz-panel or any other jdaviz attributes. I'm not sure how users would install this css other than hosting it on a CDN. But I can imagine something like including <link href="...jdaviz.css" rel="stylesheet" type="text/css" /> give users the default stylings, or they can create their custom css to override any of the elements within for full customization.

@pllim
Copy link
Contributor

pllim commented Apr 26, 2021

So, this got 3 approvals. Is this ready to go in, or does the comment from @havok2063 need addressing first?

@rosteen rosteen merged commit e81a09b into spacetelescope:main Apr 27, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants