Skip to content

Comments

add scoped to all ncappnavigation components#4730

Merged
szaimen merged 2 commits intomasterfrom
enh/noid/make-components-scoped
Nov 2, 2023
Merged

add scoped to all ncappnavigation components#4730
szaimen merged 2 commits intomasterfrom
enh/noid/make-components-scoped

Conversation

@szaimen
Copy link
Contributor

@szaimen szaimen commented Oct 31, 2023

@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 👍

raimund-schluessler
raimund-schluessler previously requested changes Oct 31, 2023
View reviewed changes
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?

@szaimen szaimen force-pushed the enh/noid/make-components-scoped branch 3 times, most recently from af30380 to c2834da Compare November 1, 2023 15:52
susnux
susnux reviewed Nov 1, 2023
View reviewed changes
src/components/NcAppNavigationItem/NcAppNavigationItem.vue
}
}
<style scoped lang="scss">
@import '../../assets/NcAppNavigationItem';
Copy link
Contributor

@susnux susnux Nov 1, 2023

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

@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.

done

Copy link
Contributor Author

@szaimen szaimen Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I tried this and it didnt work: See 6c66898 and https://github.com/nextcloud-libraries/nextcloud-vue/actions/runs/6734727700/job/18306263029

@szaimen szaimen requested a review from susnux November 2, 2023 15:27
@szaimen szaimen force-pushed the enh/noid/make-components-scoped branch from 6c66898 to c0b18dd Compare November 2, 2023 15:30
JuliaKirschenheuter
JuliaKirschenheuter approved these changes Nov 2, 2023
View reviewed changes
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
@szaimen szaimen deleted the enh/noid/make-components-scoped branch November 2, 2023 17:51
@skjnldsv skjnldsv mentioned this pull request Nov 3, 2023
Merged
@susnux susnux mentioned this pull request Nov 8, 2023
Merged
2 tasks
@raimund-schluessler raimund-schluessler mentioned this pull request Jan 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@susnux susnux susnux approved these changes

@JuliaKirschenheuter JuliaKirschenheuter JuliaKirschenheuter approved these changes

@raimund-schluessler raimund-schluessler Awaiting requested review from raimund-schluessler

@nfebe nfebe Awaiting requested review from nfebe

@Pytal Pytal Awaiting requested review from Pytal

@ShGKme ShGKme Awaiting requested review from ShGKme

Assignees

No one assigned

Labels

3. to review Waiting for reviews bug Something isn't working

Projects

None yet

Milestone

8.0.0

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

@szaimen @ShGKme @susnux @raimund-schluessler @JuliaKirschenheuter