Skip to content

Conversation

@pixelipo
Copy link
Contributor

@pixelipo pixelipo commented Nov 21, 2017

While working on cleaning up @nextcloud/gallery iconset, I noticed that the .icon-close SVG is disproportionately smaller. It turns out it had a lot more empty space in SVG than other icons. This is only visible when that icon is placed inline with another icon, as is the case in the gallery app.

EDIT: outdated screenshots; look at next comment
Old close.svg had more than 3px margin within SVG; after this PR, it will have 1px. Here's a before:
before

before0

and after:
after

after0

Please notice that in the searchbox, icon gets bigger. @nextcloud/designers is this OK? If not, let me know so I can add:
background-size: 12px;
in styles.scss (line 202)

P.S. I think in the sidebar, this icon should be as large as it is in this PR, because it doesn't make sense for the most-clicked icon to be smaller than purely visual ones.

Signed-off-by: Marin Treselj <marin@pixelipo.com>
@pixelipo pixelipo added 3. to review Waiting for reviews design Design, UI, UX, etc. labels Nov 21, 2017
@pixelipo pixelipo mentioned this pull request Nov 21, 2017
14 tasks
Signed-off-by: Marin Treselj <marin@pixelipo.com>
@pixelipo
Copy link
Contributor Author

Sorry, I think I went overboard with the initial commit; I've pushed a smaller version version, here's a new before/after:
image
image

@codecov
Copy link

codecov bot commented Nov 21, 2017

Codecov Report

Merging #7239 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #7239   +/-   ##
=========================================
  Coverage     50.84%   50.84%           
  Complexity    24548    24548           
=========================================
  Files          1585     1585           
  Lines         93804    93804           
  Branches       1354     1354           
=========================================
  Hits          47698    47698           
  Misses        46106    46106

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

I like it 👍

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Good catch! :)

@jancborchardt jancborchardt merged commit 968da5e into master Nov 22, 2017
@jancborchardt jancborchardt deleted the larger-close-button branch November 22, 2017 13:01
@skjnldsv
Copy link
Member

👍

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 design Design, UI, UX, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants