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.

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

Jump to bottom

Fix icon repeat and position for AppNavigationItem #3539

Merged
merged 1 commit into from
Dec 2, 2022
Merged

Fix icon repeat and position for AppNavigationItem #3539

merged 1 commit into from
Dec 2, 2022

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Dec 2, 2022 •
edited
Loading

I don't really understand how no one experienced this before...

EDIT: ah, ok.
Some icons do not start with icon-
image

Before After
2022-12-02_12-04 2022-12-02_12-04_1

@skjnldsv
Fix icon repeat and position for AppNavigationItem
3364415 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv skjnldsv added bug Something isn't working 3. to review Waiting for reviews feature: app-navigation Related to the app-navigation component regression Regression of a previous working feature labels Dec 2, 2022
@skjnldsv skjnldsv added this to the 7.1.1 milestone Dec 2, 2022
@skjnldsv skjnldsv self-assigned this Dec 2, 2022
@skjnldsv skjnldsv requested review from artonge, Pytal and szaimen and removed request for a team December 2, 2022 11:06
@raimund-schluessler
Copy link
Contributor

I don't really understand how no one experienced this before...

Maybe everyone ideally uses material design icons 😉 🙈

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Dec 2, 2022

Maybe everyone ideally uses material design icons wink see_no_evil

did find this on legacy stuff, so yeah... 🙈

@szaimen
Copy link
Contributor

szaimen commented Dec 2, 2022

Why is the css not scoped?

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Dec 2, 2022

Why is the css not scoped?

No clue, not relevant to this PR 🙈

@szaimen
Copy link
Contributor

szaimen commented Dec 2, 2022

@skjnldsv are you sure that it is unrelated? wouldn't scoping the css not disable the impact on legacy components or do I understand something incorrectly?

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Dec 2, 2022 •
edited
Loading

I understand something incorrectly?

Yes, this is not the issue. The issue is those two missing css rules that are implied because icons classes starts with icon-, but this rule is not true for legacy files icons. e.g. nav-icon-trashbin

Feel free to try in the vue styleguide with
<NcAppNavigationItem title="My title" icon="nav-icon-systemtagsfilter" />
image

szaimen
szaimen approved these changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Fine by me then 👍

@skjnldsv skjnldsv merged commit 288d451 into master Dec 2, 2022
@skjnldsv skjnldsv deleted the fix/app-navigation-icon-repeat branch December 2, 2022 13:12
@skjnldsv skjnldsv modified the milestones: 7.1.1, 7.2.0 Dec 6, 2022
@skjnldsv skjnldsv mentioned this pull request Dec 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raimund-schluessler raimund-schluessler raimund-schluessler approved these changes

@szaimen szaimen szaimen approved these changes

@marcoambrosini marcoambrosini Awaiting requested review from marcoambrosini

@artonge artonge Awaiting requested review from artonge

@Pytal Pytal Awaiting requested review from Pytal

Assignees

@skjnldsv skjnldsv

Labels
3. to review Waiting for reviews bug Something isn't working feature: app-navigation Related to the app-navigation component regression Regression of a previous working feature
Projects
None yet
Milestone
7.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@skjnldsv @raimund-schluessler @szaimen