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

add scoped to all ncappnavigation components #4730

Merged
merged 2 commits into from Nov 2, 2023

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented Oct 31, 2023

Signed-off-by: Simon L <szaimen@e.mail.de>
@szaimen szaimen added bug Something isn't working 2. developing Work in progress labels Oct 31, 2023
@szaimen szaimen added this to the 8.0.0 milestone Oct 31, 2023
@szaimen
Copy link
Contributor Author

szaimen commented Oct 31, 2023

This might have broken NcAppNavigationNewItem:
image

@szaimen
Copy link
Contributor Author

szaimen commented Oct 31, 2023

@ShGKme I suppose this is not the fix to the linked problem? Am I missing something?

@ShGKme
Copy link
Contributor

ShGKme commented Oct 31, 2023

@ShGKme I suppose this is not the fix to the linked problem? Am I missing something?

It should work if all nextcloud/vue on a page is updated and has scoped.

Is there another app that adds these styles on the page?

Scoping styles with scoped is only a one-way scoping, meaning it saves this component from changing other elements on the page but doesn't save this component from being overridden by styles outside.
(+1 for reliable <style module>)

@szaimen szaimen marked this pull request as ready for review October 31, 2023 16:04
@szaimen szaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 31, 2023
@szaimen
Copy link
Contributor Author

szaimen commented Oct 31, 2023

@ShGKme I suppose this is not the fix to the linked problem? Am I missing something?

It should work if all nextcloud/vue on a page is updated and has scoped.

Is there another app that adds these styles on the page?

Scoping styles with scoped is only a one-way scoping, meaning it saves this component from changing other elements on the page but doesn't save this component from being overridden by styles outside. (+1 for reliable <style module>)

All right. this is ready for review then 👍

Copy link
Contributor

@raimund-schluessler raimund-schluessler left a comment

Choose a reason for hiding this comment

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

This might have broken NcAppNavigationNewItem:

This is still an issue. Since the styles are now scoped, they need to be duplicated for NcAppNavigationNewItem to apply there.

@susnux
Copy link
Contributor

susnux commented Oct 31, 2023

Maybe share the styles as in a scss file and import as scoped styles in the components?

}
}
<style scoped lang="scss">
@import '../../assets/NcAppNavigationItem';
Copy link
Contributor

Choose a reason for hiding this comment

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

@import is deprecated you should use @use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

@szaimen szaimen Nov 2, 2023

Choose a reason for hiding this comment

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

@szaimen szaimen requested a review from susnux November 2, 2023 15:27
Signed-off-by: Simon L <szaimen@e.mail.de>
@szaimen szaimen force-pushed the enh/noid/make-components-scoped branch from 6c66898 to c0b18dd Compare November 2, 2023 15:30
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

works!

@szaimen szaimen merged commit acabba2 into master Nov 2, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BITV]: Styles of left sidebar will be changed as far right sidebar will be opened
5 participants