Skip to content

Disable the injected snapper logic when apps want to ship their own - #14972

Merged
rullzer merged 1 commit into
masterfrom
fix/app-nav-toggle-missing
Apr 10, 2019
Merged

Disable the injected snapper logic when apps want to ship their own#14972
rullzer merged 1 commit into
masterfrom
fix/app-nav-toggle-missing

Conversation

@ChristophWurst

@ChristophWurstChristophWurst commented Apr 5, 2019•
edited
Loading

Copy link
Copy Markdown
Member

This allows apps to ship their own, as in some cases the #app-content
element does not exist on page load and therefore the injection fails
and the icon is missing afterwards.

With nextcloud-libraries/nextcloud-vue#339 we'll ship our own Vue-specific logic to handle the sliding.

Fixes#14956

@ChristophWurstChristophWurst added this to the Nextcloud 16 milestone Apr 5, 2019
@ChristophWurstChristophWurst self-assigned this Apr 5, 2019
@ChristophWurstChristophWurst mentioned this pull request Apr 5, 2019
Closed
@ChristophWurstChristophWurst added 1. to develop Accepted and waiting to be taken care of and removed 3. to review Waiting for reviews labels Apr 5, 2019
@ChristophWurst

ChristophWurst commented Apr 5, 2019

Copy link
Copy Markdown
MemberAuthor

ChristophWurst added 1. to develop and removed 3. to review labels just now

Put on hold as long as I don't know if this really works.

skjnldsv
skjnldsv reviewed Apr 5, 2019
View reviewed changes
Comment threadcore/js/js.js Outdated
@ChristophWurst
ChristophWurstforce-pushed the fix/app-nav-toggle-missing branch from 3b6ee4b to 0687d17CompareApril 9, 2019 09:18
@ChristophWurstChristophWurst mentioned this pull request Apr 10, 2019
Merged
@ChristophWurst
ChristophWurstforce-pushed the fix/app-nav-toggle-missing branch 2 times, most recently from 0178ac5 to 708491aCompareApril 10, 2019 12:08
@ChristophWurst
Do not inject a #app-navigation-toggle if there is already one
0871b9a
This allows apps to ship their own, as in some cases the #app-content
element does not exist on page load and therefore the injection fails
and the icon is missing afterwards.
Fixes#14956
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
ChristophWurstforce-pushed the fix/app-nav-toggle-missing branch from 708491a to 0871b9aCompareApril 10, 2019 12:09
@ChristophWurstChristophWurst changed the title Do not inject a #app-navigation-toggle if there is already oneDisable the injected snapper logic when apps want to ship their ownApr 10, 2019
@ChristophWurstChristophWurst added 3. to review Waiting for reviews and removed 1. to develop Accepted and waiting to be taken care of labels Apr 10, 2019
MorrisJobke
MorrisJobke approved these changes Apr 10, 2019
View reviewed changes

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code makes sense 👍

@MorrisJobke

MorrisJobke commented Apr 10, 2019

Copy link
Copy Markdown
Member

Please document this in the developer documentation.

@MorrisJobkeMorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 10, 2019
@skjnldsv

skjnldsv commented Apr 10, 2019

Copy link
Copy Markdown
Member

@MorrisJobke this will be for the vue components only while we transition.
So not sure we need this documented. :)

@MorrisJobke

MorrisJobke commented Apr 10, 2019

Copy link
Copy Markdown
Member

@MorrisJobke this will be for the vue components only while we transition.
So not sure we need this documented. :)

Okay

@rullzerrullzer mentioned this pull request Apr 10, 2019
Merged
13 tasks
@rullzer
rullzer merged commit 47226a1 into masterApr 10, 2019
@rullzer
rullzer deleted the fix/app-nav-toggle-missing branch April 10, 2019 19:41
@MorrisJobke

MorrisJobke commented Apr 10, 2019

Copy link
Copy Markdown
Member

@ChristophWurst Backport to what branch?

@ChristophWurst

ChristophWurst commented Apr 11, 2019

Copy link
Copy Markdown
MemberAuthor

at least to 15. @skjnldsv opinions?

@ChristophWurst

ChristophWurst commented Apr 11, 2019

Copy link
Copy Markdown
MemberAuthor

/backport to stable15

@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Apr 11, 2019
Merged
@backportbot-nextcloud

backportbot-nextcloudBot commented Apr 11, 2019

Copy link
Copy Markdown

backport to stable15 in #15046

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MorrisJobkeMorrisJobkeMorrisJobke approved these changes
@skjnldsvskjnldsvskjnldsv approved these changes
@rullzerrullzerAwaiting requested review from rullzer
@juliusknorrjuliusknorrAwaiting requested review from juliusknorr
@nickvergessennickvergessenAwaiting requested review from nickvergessen

Assignees

@ChristophWurstChristophWurst

Labels

4. to releaseReady to be released and/or waiting for tests to finishbugdesignDesign, UI, UX, etc.feature: vue apps

Projects

None yet

Milestone

Nextcloud 16

Development

Successfully merging this pull request may close these issues.

4 participants

@ChristophWurst@MorrisJobke@skjnldsv@rullzer