-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 opacity and padding inconsistency in breadcrumbs #5229
Conversation
Signed-off-by: Marin Treselj <marin@pixelipo.com>
@jancborchardt could you please review or select some reviewers? Thanks. P.S. I see some CI fails, but I don't think they are related to my changes. |
Nice @pixelipo! Just a quick question: What did you use for compressing the icon? It looks like maybe SVGO, which unfortunately is lossy and leads to corrupted files: #4076 Better would be to use Scour, we have a script line you can simply copy for that at https://github.com/nextcloud/server/blob/master/core/img/image-optimization.sh#L10 (I meant to mention that in the original issue, sorry I forgot. :) |
@jancborchardt I used no tool for compression - just cleanup by hand of unneeded Inkscape cruft. The only thing that might be causing problems is spaces I removed after If you're worrying about missing decimals, that because there are none - I redraw the icon in Inkscape and made sure all the corners are at full pixels 😉 |
Yeah, I also drew the icons at full pixels always of course. :) Can you make sure you compress via Scour? Then it's not a manual process but repeatable for all icons. |
Signed-off-by: Marin Treselj <marin@pixelipo.com>
Codecov Report
@@ Coverage Diff @@
## master #5229 +/- ##
============================================
- Coverage 53.98% 53.89% -0.09%
+ Complexity 22466 22350 -116
============================================
Files 1390 1318 -72
Lines 85980 78032 -7948
Branches 1329 0 -1329
============================================
- Hits 46418 42059 -4359
+ Misses 39562 35973 -3589
|
@jancborchardt ok, I see the point in repeateable processes. I have ran I would however, suggest changes to image-optimization.sh: |
@pixelipo nice, please add a commit with the suggested changes to image-optimization.sh :) |
@pixelipo I invited you to the Nextcloud Github organization some time ago, you can accept at https://github.com/nextcloud :) |
Signed-off-by: Marin Treselj <marin@pixelipo.com>
@jancborchardt this should fix both issues. N.B. I've set |
My suggestion for further SVG optimiztion - as mentioned in #5229 Signed-off-by: Marin Treselj <marin@pixelipo.com>
@pixelipo Is this ready for review? Please change the according label from 2. developing to 3. to review then. 😉 |
yes, @juliushaertl - ready for review. Please check whether issues mentioned by @jancborchardt were fixed |
@pixelipo I think the text should be alligned in the middle of the arrow separators, not at the baseline. It looks a bit out of place at the moment. Also the whitespace between the sharing icon lstill ooks to big. |
Signed-off-by: Marin Treselj <marin@pixelipo.com>
I have fixed both issues now, @juliushaertl @jancborchardt - this is ready for review (and merge, I hope). Text issue was fixed with line height and the icons issue was fixed by - what I think is the best solution for this use case - making icon relatively positioned and moving them over crumb title's padding. |
3 JSUnit tests fail:
|
Yup, to confirm @juliushaertl's comment - the text was positioned too low on that screenshot. :) |
@pixelipo can you check out the JS unit test failures? |
I didn't have a working dev setup until today :) I'll take a look tomorrow, OK? |
Sure, at your leisure. :) Sorry this review process takes so long, please don't be discouraged as you do great work! |
Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>
I found&fixed a small issue with padding, however, I'm unable to figure out what is the issue with JS unit tests. I did run them and got same errors as @MorrisJobke - but I get same errors when running unit tests on master, so they are not related to this PR. Everything looks fine @320px: but there is a problem @480px: This happens when current folder has a very long filename. It seems that it's not ellipsized "soon enough", but IMO it should be fixed separately from this PR P.S. I noticed a bigger issue with breadcrumbs on mobile: Please review. |
Can you open two separate issues about the two breadcrumb problem and new file popover? :) |
This pull request now seems good 👍 |
The JSUnit tests still fail. @juliushaertl @danxuliu do you mind to help @pixelipo out to fix this? |
@MorrisJobke as mentioned in #5229 (comment) , JSUnit tests fail on master as well, so they should be fixed in a separate PR. |
This could not be the case - they run fine on master: https://drone.nextcloud.com/nextcloud/server/9014/34 And the failing unit tests are even related to the width when the breadcrumbs get's visible or hidden. Please rebase on latest master and check again. On macOS the unit tests for the breadcrumbs somehow fail always :/ |
I can't fix JSUnit tests here since I'm unable to reproduce working tests on master. Here's my output when running
|
Cann you help @nextcloud/javascript? |
I worked on this issue before, can't remember when and if I pushed it somewhere here. |
@danxuliu Could you have a look at this? Getting it in would be super nice :) |
The JavaScript tests fail due to the breadcrumb items shown no longer matching those expected. However, the commit Fix opacity and padding inconsistency in breadcrumbs itself is fine; the problem comes from the JavaScript tests themselves. The As different browsers may show the items with slightly different sizes the tests for resizing use hard-coded widths for the breadcrumb items, but not for the ellipsis. For some strange reason, when using the new CSS the width returned for the ellipsis by PhantomJS is way larger than expected (242px instead of 51px); due to this there is less room for the breadcrumb items and the first one that was expected to be shown is not, thus causing the Hides breadcrumbs to fit max allowed width and Updates the breadcrumbs when reducing max allowed width tests to fail. If instead of PhantomJS the tests are run using Firefox (52.3) then the ellipsis is now shorter than before (37px instead of 50px), which also causes the Updates the breadcrumbs when reducing max allowed width test to fail because now there is enough room to show another item that was expected to be hidden. The solution? In the same way that the width of the breadcrumb items is hard-coded the width of the ellipsis has to be hardcoded too. To do that the easiest way is stub the That would solve two of the three failures. The other one, links the breadcrumb to the trashbin view, is related to that explained above, but it is not quite the same. When resizing them, the available width for the breadcrumbs is got from the value set using Again, the easiest way to solve this would be to hard-code the width of the breadcrumbs by calling @pixelipo If you want I can add a new commit to your pull request with these fixes. Besides that, could you rebase your commits onto current master and check that they are all still valid (which I am sure they are, but just in case ;-) )? Thanks! :-) |
Maybe we should rebuild the breadcrumb system & the tests? |
I'm rebuilding the breadcrumbs in #7051. Will this pr will become obsolete? The tests will be fixed. |
@skjnldsv probably. I've added myself as a reviewer to the other one to keep an eye on it ;) I'll check if it solves all the issues this PR was trying to solve. |
Part 1 of my work on cleaning up icon inconsistencies ( #5157 ).
This tackles
home.svg
as well as some minor related fixes in.breadcrumb
.Please test this across different apps. I tested Files, Deck (some code can be removed there after this is merged) and Mail (this would deprecate
.icon-inbox
icon/style). I don't know if it is used elsewhere (couldn't find usage in Contacts, Calendar, News, Tasks and Gallery).Signed-off-by: Marin Treselj marin@pixelipo.com