LBRYCommunity/lbry-desktop

Fork 0

Icon improvements #555

Closed

alexliebowitz wants to merge 15 commits from icon-improvements into master

alexliebowitz commented

2017-09-12 08:03:25 +02:00

(Migrated from github.com)

A handful of improvements related to icons in file tiles and cards:

Adds an <IconSet /> component and refactors file tiles and file cards to use it to display the standard icon group (price, featured content icon, local content icon)
Factors out the "you already have this file" folder icon into its own component
Adds a custom style to manually resize and realign the rocket logo to look better next to the folder logo. This is the only way to handle it because the misalignment is in the font glyph itself, so there's nothing we can do with CSS centering to fix it. There are loads of Font Awesome issues about this (example) and they basically say you should hack it with CSS until they fix it in FA 5.

A handful of improvements related to icons in file tiles and cards: - Adds an `<IconSet />` component and refactors file tiles and file cards to use it to display the standard icon group (price, featured content icon, local content icon) - Factors out the "you already have this file" folder icon into its own component - Adds a custom style to manually resize and realign the rocket logo to look better next to the folder logo. This is the only way to handle it because the misalignment is in the font glyph itself, so there's nothing we can do with CSS centering to fix it. There are loads of Font Awesome issues about this ([example](https://github.com/FortAwesome/Font-Awesome/issues/9563#issuecomment-251210751)) and they basically say you should hack it with CSS until they fix it in FA 5. <img width="272" alt="screen shot 2017-09-12 at 1 39 05 am" src="https://user-images.githubusercontent.com/7283989/30310324-e9379cbe-975d-11e7-9116-683c902e4a4f.png"> <img width="821" alt="screen shot 2017-09-12 at 2 16 32 am" src="https://user-images.githubusercontent.com/7283989/30310825-6d8b5260-9760-11e7-9b41-bcbe43841749.png">

kauffj (Migrated from github.com) reviewed

2017-09-12 08:03:25 +02:00

alexliebowitz (Migrated from github.com) reviewed

2017-09-12 08:06:38 +02:00

ui/js/component/common.js

alexliebowitz (Migrated from github.com) commented

2017-09-12 08:06:38 +02:00

Couple issues about names:

It could obviously also be IconPanel, IconGroup, etc. No strong preference.
Do we want a name that doesn't start with icon? Makes it easy to confuse with actual icons, and I actually had to use an underscore in the CSS class because Font Awesome selects everything starting with icon-.

Couple issues about names: 1. It could obviously also be IconPanel, IconGroup, etc. No strong preference. 2. Do we want a name that doesn't start with icon? Makes it easy to confuse with actual icons, and I actually had to use an underscore in the CSS class because Font Awesome selects everything starting with `icon-`.

alexliebowitz (Migrated from github.com) reviewed

2017-09-12 08:08:30 +02:00

ui/scss/component/_card.scss

alexliebowitz (Migrated from github.com) commented

2017-09-12 08:08:30 +02:00

As I mentioned, CSS hacks are the only option to fix the vertical centering on FA icons, but I feel like this isn't the right file. Where should it go? _gui.scss? Its own component stylesheet?

As I mentioned, CSS hacks are the only option to fix the vertical centering on FA icons, but I feel like this isn't the right file. Where should it go? `_gui.scss`? Its own component stylesheet?

kauffj (Migrated from github.com) requested changes

2017-09-12 22:35:04 +02:00

kauffj (Migrated from github.com) left a comment

Some questions about this.

ui/js/component/common.js

kauffj (Migrated from github.com) commented

2017-09-12 22:29:28 +02:00

IconSet is fine. Please put a comment in CSS related to the break from BEM naming pattern.

`IconSet` is fine. Please put a comment in CSS related to the break from BEM naming pattern.

ui/js/component/fileCard/view.jsx

kauffj (Migrated from github.com) commented

2017-09-12 22:28:56 +02:00

FilePrice can be inside a IconSet?

`FilePrice` can be inside a `IconSet`?

ui/js/component/iconLocal/view.jsx

kauffj (Migrated from github.com) commented

2017-09-12 22:32:09 +02:00

Did you see my comment on the previous PR raising concerns about this pattern? It seems to scale poorly.

I was suggesting that perhaps icon titles be handled by Icon itself with the titles determined by the icon class, or a constant.

Did you see my comment on the previous PR raising concerns about this pattern? It seems to scale poorly. I was suggesting that perhaps icon titles be handled by `Icon` itself with the titles determined by the icon class, or a constant.

ui/scss/_gui.scss

kauffj (Migrated from github.com) commented

2017-09-12 22:32:54 +02:00

This is a property of a card, or otherwise something like icon_set--right.

Icon set CSS can go in _icons.scss.

This is a property of a card, or otherwise something like `icon_set--right`. Icon set CSS can go in `_icons.scss`.

ui/scss/component/_card.scss

kauffj (Migrated from github.com) commented

2017-09-12 22:34:45 +02:00

My inclination would be to keep all icon related hack fixes in either _icons.scss in a clearly delineated section, or even it's own file _icon-hacks.scss or similar.

But if only the external component can adjust the spacing, I'm less sure about this design.

I'm confused about this adjustment. Is there a standardized adjustment that can be made to all rocket icons? Or does the adjustment need to be contextual based on the spacing of the parent element(s)? My inclination would be to keep all icon related hack fixes in either `_icons.scss` in a clearly delineated section, or even it's own file `_icon-hacks.scss` or similar. But if only the external component can adjust the spacing, I'm less sure about this design.

alexliebowitz (Migrated from github.com) reviewed

2017-09-12 23:06:46 +02:00

ui/scss/component/_card.scss

alexliebowitz (Migrated from github.com) commented

2017-09-12 23:06:45 +02:00

I'm using ems, I don't think it has to be contextual. I just thought of _icons.scss as being basically the stylesheet direct from FA, not something we want to modify. But that makes sense enough. I'll move the style there.

I'm using ems, I don't think it has to be contextual. I just thought of `_icons.scss` as being basically the stylesheet direct from FA, not something we want to modify. But that makes sense enough. I'll move the style there.

alexliebowitz (Migrated from github.com) reviewed

2017-09-12 23:16:23 +02:00

ui/js/component/fileCard/view.jsx

alexliebowitz (Migrated from github.com) commented

2017-09-12 23:16:23 +02:00

IconSet represents an info panel which is primarily designed to show icons but can also have other info (like a system tray which has a load of icons + the time on one side). I couldn't think of a better name for it. IndicatorPanel? I dunno.

We could also make the price completely separate from the icon set, although that will probably lead to additional presentational divs or spans to group them together wherever both a price and icons are being shown.

`IconSet` represents an info panel which is primarily designed to show icons but can also have other info (like a system tray which has a load of icons + the time on one side). I couldn't think of a better name for it. `IndicatorPanel`? I dunno. We could also make the price completely separate from the icon set, although that will probably lead to additional presentational divs or spans to group them together wherever both a price and icons are being shown.

alexliebowitz (Migrated from github.com) reviewed

2017-09-12 23:18:16 +02:00

ui/scss/_gui.scss

alexliebowitz (Migrated from github.com) commented

2017-09-12 23:18:16 +02:00

I was thinking the right justification would just be the default style and we can override it if we ever use it somewhere other than a card. But you're right, it should be either a modifier or part of IconSet.

alexliebowitz (Migrated from github.com) reviewed

2017-09-12 23:24:40 +02:00

ui/js/component/iconLocal/view.jsx

alexliebowitz (Migrated from github.com) commented

2017-09-12 23:24:40 +02:00

I saw your comment and then promptly forgot it. We were talking about a lot of small fiddly changes to icons, so it went in one ear and out the other.

It would make sense for me to do that change in this PR. I would think that constants would be better, since it moves the logic into props where it's easy to track. Inspecting CSS classes doesn't feel very React/Redux-y.

I saw your comment and then promptly forgot it. We were talking about a lot of small fiddly changes to icons, so it went in one ear and out the other. It would make sense for me to do that change in this PR. I would think that constants would be better, since it moves the logic into props where it's easy to track. Inspecting CSS classes doesn't feel very React/Redux-y.

kauffj (Migrated from github.com) reviewed

2017-09-13 15:52:12 +02:00

ui/js/component/fileCard/view.jsx

kauffj (Migrated from github.com) commented

2017-09-13 15:52:12 +02:00

Why does this exist at all actually, if it doesn't have styles? (other than the float, which is really a property of the card)

kauffj (Migrated from github.com) reviewed

2017-09-13 15:52:29 +02:00

ui/js/component/iconLocal/view.jsx

kauffj (Migrated from github.com) commented

2017-09-13 15:52:29 +02:00

Ok, please refactor this.

alexliebowitz (Migrated from github.com) reviewed

2017-09-15 07:36:41 +02:00

ui/js/component/fileCard/view.jsx

alexliebowitz (Migrated from github.com) commented

2017-09-15 07:36:41 +02:00

I suppose it doesn't need to exist anymore. Unless we wanted to change it to a fancy component that displays the various indicators via props. I'll switch it back to a div or span, with the float: right handled by card level styles.

lyoshenka commented

2017-09-19 15:27:15 +02:00

(Migrated from github.com)

@alexliebowitz in addition to changing the pipeline, you have to re-assign jeremy as a reviewer if you want him to re-review your changes. otherwise it won't show up in his list.

kauffj commented

2017-09-22 23:51:36 +02:00

(Migrated from github.com)

Squashed and merged in 861241ca3a

Additional changes by me in 0a4e17af54

I thought it was simpler to just have the const definition match the rendered icon and I didn't see a necessity to LBRY having it's own class icon class scope.

Squashed and merged in https://github.com/lbryio/lbry-app/commit/861241ca3a8a9e0ebb8bf9d57c1fda6515962322 Additional changes by me in https://github.com/lbryio/lbry-app/commit/0a4e17af5426420c5db94a334dfa5d57f1ff2e58 I thought it was simpler to just have the const definition match the rendered icon and I didn't see a necessity to LBRY having it's own class icon class scope.