-
Notifications
You must be signed in to change notification settings - Fork 39
fix: id of content to be unique #475
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
Conversation
ee53f8e to
67934c8
Compare
`content` is already used as an id in the server template: https://github.com/nextcloud/server/blob/c10317f7f9f53a4de464915f754896eed7f1ee6c/core/templates/layout.user.php#L125 Make sure to have a unique id and to replace the correct dom element when mounting the vue app. The app uses NcAppContent which is rendered as a `<main>` tag. Mount the app on `main#content` from the server template so there is only one `<main>` on the page. Signed-off-by: Max <max@nextcloud.com>
67934c8 to
76e3676
Compare
|
I replaced the |
| @@ -1 +1,8 @@ | |||
| <div id="content"></div> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well this element is replaced anyway on mounting
I had a failure in my comment. Why actually should be done is:
https://github.com/nextcloud/app-tutorial/blob/master/src/App.vue#L2
Should use NcContent
https://nextcloud-vue-components.netlify.app/#/Components/App%20containers?id=nccontent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about using NcContent - I thought I'd do that in another PR.
But I think the element that is replaced on mounting is actually this:
https://github.com/nextcloud/server/blob/master/core/templates/layout.user.php#L125
It's a parent element so the div#content will also be removed.
I'm not sure what vue will do if the id is used twice. What i am trying to do here is to avoid that ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think the element that is replaced on mounting is actually this:
https://github.com/nextcloud/server/blob/master/core/templates/layout.user.php#L125
Which is exactly why your PR is "wrong". That element must not be removed/replaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh wow... but then we cannot use NcAppContent anymore because
A document must not have more than one main element that does not have the hidden attribute specified.
https://html.spec.whatwg.org/multipage/grouping-content.html#the-main-element
Or rather we should turn that main element into a div and use a different id then content on it. Which would basically revert nextcloud/server@a8ff2ff#diff-190a82b500ef1394d8a0cb94acb6225c1416a9d7d6340f08432da49fc5c98eba . But that was introduced on purpose as well. So looks like there's no easy solution here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm right, seems like talk also as an empty template:
https://github.com/nextcloud/spreed/blob/master/templates/index.php
I thought we always nest. But then it seems your current patch is fine.
|
Closing as stale ;) |


contentis already used as an id in the server template: https://github.com/nextcloud/server/blob/c10317f7f9f53a4de464915f754896eed7f1ee6c/core/templates/layout.user.php#L125Make sure to have a unique id and to replace the correct dom element when mounting the vue app.
Fixes #474.