Skip to content

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.

Sign up for GitHub

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

Closed
max-nextcloud wants to merge 1 commit into from
Closed

fix: id of content to be unique #475

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion templates/main.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,8 @@
<div id="content"></div>
Copy link
Member

@nickvergessen nickvergessen Feb 10, 2023

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

Copy link
Author

@max-nextcloud max-nextcloud Feb 10, 2023

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.

Copy link
Member

@nickvergessen nickvergessen Feb 10, 2023

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.

Copy link
Author

@max-nextcloud max-nextcloud Feb 10, 2023 •
edited
Loading

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.

Copy link
Author

@max-nextcloud max-nextcloud Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That element must not be removed/replaced.

But that's already happening anyway.

Here's the dom in current master branch before the vue mount:
Bildschirmfoto vom 2023-02-10 11-13-03

And this is the dom afterwards:
grafik

Copy link
Member

@nickvergessen nickvergessen Feb 10, 2023

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.

<!--
This is an empty template on purpose.
Nextcloud server renders it inside the default layout
in the <main id="content"> tag.

The vue app created in src/main.js replaces the main tag.
Therefore any content provided here will be overwritten.
-->