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

Fix icons from settings page disappears when nav item overflows #24378

Closed
wants to merge 8 commits into from

Conversation

olivierperdaens
Copy link

Fixing #23849 bug.
Issue was general for the settings page: when a navigation item text overflows, svg icons disapeared.
Some minor changes in the apps.scss file fixed the issue.

How to test:

  1. Go on the settings page
  2. Inspect the code and add text in the navigation item
  3. See the icon stays and the overflow is well handled

@jancborchardt
Copy link
Member

Hi @olivierperdaens, thanks for your contribution! :) It does fix the vanishing icon, but it causes all the entries to be shifted to the right:

Before (correct text alignment) After (indented entries and headings too far left)
overflow before overflow after

@jancborchardt
Copy link
Member

@ma12-co @juliushaertl isn’t this something we have a Vue component for we should use here btw?

@skjnldsv
Copy link
Member

skjnldsv commented Nov 30, 2020

@ma12-co @juliushaertl isn’t this something we have a Vue component for we should use here btw?

No, the settings is not migrated to vue, (and will most likely never be btw :p)

@olivierperdaens
Copy link
Author

@jancborchardt Indeed, it creates a shifting. This is because, the margin are also used for the navigation-bar on the main page. And on that page, the svg logo are prepended to the "nav-item". Which is different for the settings page where the svg logo is "part" of the "nav-item".

I tryed to understand how the main page's navigation-bar was generated but I couldn't completely understand it (it is handled in a different way, and as it is my first contribution I still don't have a global vue on the project).

I can dig deeper to try solving the vanishing icons without changing the margin on the setting's navigation-bar !

Main page Settings page
Capture d’écran 2020-11-30 à 15 36 32 Capture d’écran 2020-11-30 à 15 40 57

@olivierperdaens
Copy link
Author

@jancborchardt I keep searching this afternoon and I came to a fix.
It seems that the icon bug is solved without changing the "alignment" of the navigation bar in the setting's page.

I'll let you check and come back to me if there is anything more to change !

@@ -259,6 +261,9 @@ kbd {
}
}
/* Main entry link */
.settings_nav_item{
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, no, those fixes are global.
The apps.scss must not contain any settings specific changes.
You can test yourself the available html options for the navigation menu.
The img needs to be fixed instead https://docs.nextcloud.com/server/stable/developer_manual/design/navigation.html#app-navigation-menu

Copy link
Author

Choose a reason for hiding this comment

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

Ok, sorry. I'll try to fix this in another way !

Copy link
Author

@olivierperdaens olivierperdaens Dec 25, 2020

Choose a reason for hiding this comment

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

Hi @skjnldsv ! I worked on fixing the img css instead of creating a specific class for the settings'navigation menu (as you advised me) and I came to a satisfying result. Please let me know if it's better or if it still needs to be done another way.

Comment on lines +281 to +286
position: absolute;
top: 15px;
margin-right: 11px;
width: 16px;
height: 16px;
margin-left: -30px;
margin-left: -30px;
Copy link
Member

Choose a reason for hiding this comment

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

Indentation is off :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants