Uh oh!
There was an error while loading. Please reload this page.
Vue offline - #1962
Conversation
…udio into vue-offline # Conflicts: # contentcuration/contentcuration/frontend/shared/views/channel/ChannelModal.vue
Codecov Report
@@ Coverage Diff @@## vue-refactor #1962 +/- ##
=============================================
Coverage 78.87% 78.87% =============================================
Files 274 274 Lines 13659 13659 =============================================
Hits 10773 10773 Misses 2886 2886 Continue to review full report at Codecov.
|
jayoshih
commented
Jun 18, 2020
Looks like the suites are passing, Travis just hasn't detected that it's done |
rtibbles
left a comment
There was a problem hiding this comment.
Two things for clarification - the need to repeatedly offset and position, and the HTML scroll behaviours.
The actual offline indicator stuff looks great, and has a good balance between being insertable anywhere it is needed, but also being present by default wherever possible.
| }, | ||
| }, | ||
| watch: { | ||
| value(val) { |
There was a problem hiding this comment.
Are we generally controlling the display of FullscreenModals via the values prop, or more often via a route?
Might be good to add a destroy handler here that calls this.hideHTMLScroll(false) in case it is just being cleaned up as part of a client side navigation.
There was a problem hiding this comment.
I had tried that originally, but there were some difficulties with using the keep-alive option on the router-view tags. Should we avoid using this attribute in general?
There was a problem hiding this comment.
Oh right - so what we really want is a 'when rendered' handler. Going to take a look at what the equivalent work flow is when you are using the keep-alive property.
There was a problem hiding this comment.
Ahh... we should use the activated and deactivated life cycle hooks: https://vuejs.org/v2/api/#keep-alive
These will be called whenever the kept alive component is brought back into use, and then when shut down!
| </VTabs> | ||
| </template> | ||
| </VToolbar> | ||
| <OfflineText toolbar :offset="topToolbarHeight" /> |
There was a problem hiding this comment.
Hrm - these offsets feel a bit odd to me, is this a Vuetify issue that the components would overlap each other otherwise? Seems like an odd design decision on their part.
There was a problem hiding this comment.
So I tried to use their app attribute, but the problem there is that it affects the layout of the main page because it's used on a modal (everything outside of the modal also shifts to the sizing on the modal). This is sort of a way to get the same behavior without causing problems (also some weird handling because there are multiple toolbars on the page)
There was a problem hiding this comment.
Yeah, I think this may be where my decision to use nested routes is not helping us at all. I think we had tentatively decided to go back on this previously, but don't think we've actually acted on it.
If each full screen modal was its own independent route, then it wouldn't have the same issue of the app attribute causing issues on the page it is covering.
There was a problem hiding this comment.
That would be nice! Next pass :)
Description
Added offline indicators to all pages/modals (except for import). Since it touches all the pages, it's best to get this PR merged first #1952
Screenshots (if applicable)
Channel edit modal
Channel list
Channel collection modal
Move modal
Trash modal
Edit modal
Settings
Steps to Test
Implementation Notes (optional)
Does this introduce any tech-debt items?
Noticed that there's a lot to update on the import modal, so will address that in a separate PR
Checklist
docslabel been added if this introduces a change that needs to be updated in the user docs?CHANGELOGlabel been added to this pull request? Items with this label will be added to the CHANGELOG at a later timenotranslateclass been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)?