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 svg icons disapearing in app navigation when text overflows #27936

Merged
merged 1 commit into from
Jul 13, 2021
Merged

Fix svg icons disapearing in app navigation when text overflows #27936

merged 1 commit into from
Jul 13, 2021

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Jul 13, 2021 •
edited by szaimen

The issue is caused by the icon being positionned with negative margins
and the overflow: hidden rule when hide the icon when the text
overflows. Remove positioning with negative margins.

This fix #23849
Closes #24378

szaimen
szaimen requested changes Jul 13, 2021
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment •
edited

Choose a reason for hiding this comment

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

Hm... I couldn't reproduce the initial issue on NC22.0.0 also without this fix...
image

@CarlSchwan
Copy link
Member Author

This is with a local instance and using git master (with Firefox)

image

On Chromium without this patch it's also working fine.

szaimen
szaimen approved these changes Jul 13, 2021
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.

You are right. I've overlooked that this only happens with Firefox.
Tested the PR successfully with Firefox and Chromium

@szaimen szaimen added this to the Nextcloud 23 milestone Jul 13, 2021
@szaimen
Copy link
Contributor

szaimen commented Jul 13, 2021 •
edited

Failing drone is unrelated

julien-nc
julien-nc approved these changes Jul 13, 2021
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Yup, tested with Firefox 89.

I think this can be backported to stable20, stable21 and stable22 as the issue seems to be the same. I didn't try to apply this commit in those branches though.

@szaimen
Copy link
Contributor

szaimen commented Jul 13, 2021

@CarlSchwan do you mind signing off your commits to pass the DCO check?
See https://github.com/nextcloud/server/pull/27936/checks?check_run_id=3055830036 for instructions.
Thank you!

@CarlSchwan
Fix svg icons disapearing in app navigation when text overflows
d0afc49 
The issue is caused by the icon being positionned with negative margins
and the `overflow: hidden` rule when hide the icon when the text
overflows. Remove positioning with negative margins. This was only
happening in Firefox.

This fix nextcloud#23849

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan
Copy link
Member Author

@CarlSchwan do you mind signing off your commits to pass the DCO check?
See https://github.com/nextcloud/server/pull/27936/checks?check_run_id=3055830036 for instructions.
Thank you!

Done :) and added format.signoff to my git configs so that I don't have to think about it next time :)

@szaimen
Copy link
Contributor

szaimen commented Jul 13, 2021

Great! Thanks a lot! :)

@CarlSchwan CarlSchwan mentioned this pull request Jul 13, 2021
Closed
@szaimen szaimen merged commit 2b623a4 into nextcloud:master Jul 13, 2021
@welcome
Copy link

welcome bot commented Jul 13, 2021

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@szaimen
Copy link
Contributor

szaimen commented Jul 13, 2021

/backport to stable22

@szaimen
Copy link
Contributor

szaimen commented Jul 13, 2021

/backport to stable21

@szaimen
Copy link
Contributor

szaimen commented Jul 13, 2021

/backport to stable20

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jul 13, 2021
Merged
This was referenced Jul 13, 2021
Merged
Merged
@szaimen
Copy link
Contributor

szaimen commented Jul 14, 2021

I fear this PR has broken the files app:
Are you able to reproduce?
image

@julien-nc
Copy link
Member

Damn, yes I can see it too. We couldn't see it because of the style caching...
@CarlSchwan ^^

@CarlSchwan
Copy link
Member Author

Hmm I can't reproduce with firefox but I can with chromium. On it

@julien-nc
Copy link
Member

I can reproduce with both FF and Chromium. Did you try to reload without using the cache (just in case: CTRL+F5)?

CarlSchwan added a commit that referenced this pull request Jul 14, 2021
@CarlSchwan
Fix regression in file sidebar
5cca1ac 
See #27936
@CarlSchwan CarlSchwan mentioned this pull request Jul 14, 2021
Merged
@CarlSchwan
Copy link
Member Author

I created a patch that hopefully fix the issue (it does for me): https://github.com/nextcloud/server/pull/27973/files

CarlSchwan added a commit that referenced this pull request Jul 14, 2021
@CarlSchwan
Fix regression in file sidebar
0b931b5 
See #27936

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
backportbot-nextcloud bot pushed a commit that referenced this pull request Jul 14, 2021
@CarlSchwan @backportbot
Fix regression in file sidebar
295d599 
See #27936

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
backportbot-nextcloud bot pushed a commit that referenced this pull request Jul 14, 2021
@CarlSchwan @backportbot
Fix regression in file sidebar
f223095 
See #27936

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
backportbot-nextcloud bot pushed a commit that referenced this pull request Jul 14, 2021
@CarlSchwan @backportbot
Fix regression in file sidebar
a3ee7db 
See #27936

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@anoymouserver anoymouserver mentioned this pull request Jul 31, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julien-nc julien-nc julien-nc approved these changes

@szaimen szaimen szaimen approved these changes

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@nimishavijay nimishavijay Awaiting requested review from nimishavijay

@artonge artonge Awaiting requested review from artonge

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement feature: settings
Projects
None yet
Milestone
Nextcloud 23
Development

Successfully merging this pull request may close these issues.

Firefox - User Settings - Some svg icons not visible
3 participants
@CarlSchwan @szaimen @julien-nc