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

Remove unneeded styles: everything is handled by NcVue #42559

Conversation

JuliaKirschenheuter
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter commented Jan 3, 2024

Summary

Vue-navigation Unchanged non-vue navigation
Screenshot from 2024-01-03 15-31-17 Screenshot from 2024-01-03 15-31-12

Checklist

@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/41699-change_styles_for_focused_items_in_sidebar branch from 0921b8b to 024d291 Compare January 3, 2024 15:25
@ShGKme
Copy link
Contributor

ShGKme commented Jan 3, 2024

This PR changes styles for non-@nextcloud-vue navigation while linked PR in nextcloud-vue changes @nextcloud/vue navigation. Are they related?

yes ;(

Peek 2024-01-03 17-06

Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

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

I checked this on my dev env and saw that removing this did not cause anything big to stylistically change - it looks good to me.

I do not know if there are any other apps that somehow depend on this, but to my understanding this only lets the elements that are for app navigation within a page have their border color be the primary color. When the changes to the NcAppNavigationItem.scss gets merged, this will be all good 👍

@ShGKme
Copy link
Contributor

ShGKme commented Jan 5, 2024

This PR changes styles for non-@nextcloud-vue navigation while linked PR in nextcloud-vue changes @nextcloud/vue navigation. Are they related?

So, these styles are not supposed to affect @nextcloud/vue/NcAppNavigation. But it does, because the layout of the NcAppNavigation was changed and the selector here is not actual anymore.

So the best could be to fix the selector.

but probably these styles are not actual anymore. Need to check if we still have any non-NcAppNavigation navigation.

@ShGKme
Copy link
Contributor

ShGKme commented Jan 5, 2024

I haven't found non-vue navigation anywhere. Should be safe to remove.

@JuliaKirschenheuter JuliaKirschenheuter merged commit ac10ea4 into master Jan 5, 2024
41 checks passed
@JuliaKirschenheuter JuliaKirschenheuter deleted the fix/41699-change_styles_for_focused_items_in_sidebar branch January 5, 2024 16:13
@JuliaKirschenheuter
Copy link
Contributor Author

/backport to stable28

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants