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

feat: update to ipyvuetify 1.0 (Vuetify 2.0) #132

Merged
merged 4 commits into from
Sep 26, 2019

Conversation

mariobuikhuizen
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Sep 24, 2019

Codecov Report

Merging #132 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #132      +/-   ##
=========================================
+ Coverage    92.2%   92.2%   +<.01%     
=========================================
  Files          80      80              
  Lines        3233    3234       +1     
=========================================
+ Hits         2981    2982       +1     
  Misses        252     252
Impacted Files Coverage Δ
glue_jupyter/common/toolbar_vuetify.py 80% <100%> (ø) ⬆️
glue_jupyter/widgets/subset_select_vuetify.py 88.09% <100%> (+0.29%) ⬆️
glue_jupyter/vuetify_layout.py 92.59% <100%> (ø) ⬆️
glue_jupyter/widgets/subset_mode_vuetify.py 78.04% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b973d0...c0d9f8a. Read the comment docs.

@maartenbreddels maartenbreddels self-requested a review September 24, 2019 15:18

drawer = v.NavigationDrawer(v_model=False, absolute=True, right=True,
children=[sidebar_button,
options_panel], width=350)
options_panel], width=373)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this magic number? should we add a comment in the code?

Copy link
Contributor

@nmearl nmearl left a comment

Choose a reason for hiding this comment

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

I tested this with the W5 notebook. However, I'm getting some weird behavior with the XOR selection option. Mainly, after a selection has been made, when I click somewhere else on the plot, the selection disappears. Perhaps @astrofrog knows a bit more, but it seems like the selection behavior is active even for that single click outside the region.

Sep-24-2019 11-53-10

Also, I know this isn't glue-jupyter specific, but I'd like to say that there is a certain awkwardness to the scoped slots behavior... having two children lists feels weird to me. I have some ideas from a user perspective, but I can open a discussion issue on ipyvuetify.

Otherwise, looks good!

@maartenbreddels
Copy link
Collaborator

Perhaps @astrofrog knows a bit more, but it seems like the selection behavior is active even for that single click outside the region.

I checked, but this behavior is already in master.

Also, I know this isn't glue-jupyter specific, but I'd like to say that there is a certain awkwardness to the scoped slots behavior... having two children lists feels weird to me.

It's ipyvue specific. I agree that is awkward, but I have to say that scoped slots are awkward by itself, and putting them as template-children (at the same level as the content of e.g. v-menu) is also quite unintuitive.

Our reasoning was the following. Either we completely model it like vue, so scoped slots would be children, wrapped by a v.Template(..), but that is quite verbose. Also, the only reason I can think of having scoped slots as children in a vue template, is because there is no other natural way in xml/html syntax, while in Python there would be other ways.

The way we implemented scoped slots in ipvue 1.0, is (we think) more Pythonic. But, if you have better ideas we are all ears, if we can make it even easier, that would be a win for all.

Given that this PR doesn't change behaviour, and upgrades only to ipyvuetify1.0/vuetify2, I think we can merge this.

This css feature is not yet supported on Edge, but soon will be
when Edge will be based on Chromium.
Copy link
Member

@astrofrog astrofrog 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, thanks!

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