Changes and improvements #2102

2018-11-12T21:38:40+01:00

NetOpWibby commented

2018-11-12 21:38:40 +01:00

(Migrated from github.com)

PR Checklist

This PR introduces breaking changes and I have provided a detailed explanation below.

PR Type

What kind of change does this PR introduce?

Breaking changes (bugfix or feature that introduces breaking changes)
Refactoring (no functional changes)

Fixes

I borked https://github.com/lbryio/lbry-desktop/pull/2053 while attempting to have a sane rebase. This PR refactors the CSS and makes semantic changes in the rendered HTML.

Closes #1976.

What is the current behavior?

Nothing the average user would notice, aside from styling.

What is the new behavior?

New hotness. Sass is (more) organized and easier to manage. It still isn't perfect but it's close.

## PR Checklist - [x] This PR introduces breaking changes and I have provided a detailed explanation below. ## PR Type What kind of change does this PR introduce? - [x] Breaking changes (bugfix or feature that introduces breaking changes) - [x] Refactoring (no functional changes) ## Fixes I borked https://github.com/lbryio/lbry-desktop/pull/2053 while attempting to have a sane rebase. This PR refactors the CSS and makes semantic changes in the rendered HTML. Closes #1976. ## What is the current behavior? Nothing the average user would notice, aside from styling. ## What is the new behavior? New hotness. Sass is (more) organized and easier to manage. It still isn't perfect but it's close.

neb-b (Migrated from github.com) requested changes

2018-11-13 00:15:40 +01:00

neb-b (Migrated from github.com) left a comment

This is looking great in the app but there are a few changes I think we should make that will help future development.

Also posting images with some things I found while testing.

This is looking great in the app but there are a few changes I think we should make that will help future development. Also posting images with some things I found while testing.

src/renderer/component/common/credit-amount.jsx

					
				@ -77,14 +75,11 @@ class CreditAmount extends React.PureComponent<Props> {

				    return (

				      <span

Any reason for leaving these?

src/renderer/component/dateTime/view.jsx

Keep these commented out. We are estimating the publish time based on block height.

src/renderer/component/fileDetails/view.jsx

These should use the same syntax media__info-title

These should use the same syntax `media__info-title`

src/renderer/component/header/view.jsx

Same here.

toplevelitem__sub-item--modifier

Same here. `toplevelitem__sub-item--modifier`

src/renderer/page/file/view.jsx

					
				@ -163,90 +159,85 @@ class FilePage extends React.Component<Props> {

				    return (

				      <Page forContent>

Whats the difference between media__xxx and card__xxx

I see:
media__title/card__title
media__subtitle/card__subtitle`

Can these be combined?

Whats the difference between `media__xxx` and `card__xxx` I see: `media__title`/`card__title` `media__subtitle`/card__subtitle` Can these be combined?

This modifier shoud also be something like --large instead of --file-page.

This modifier shoud also be something like `--large` instead of `--file-page`.

src/renderer/page/fileListDownloaded/view.jsx

This needs to stay. Current sorting is broken

src/renderer/scss/component/_button.scss

This shouldn't be inside of the button class. If we have to keep it, it should be a modifier on the .btn class

This shouldn't be inside of the button class. If we have to keep it, it should be a modifier on the `.btn` class

After looking over this code more, I think we should move back to variables. I think it will be hard to keep consistency with this new pattern. (And add new theme options if we ever decided to do so).

src/renderer/scss/component/_card.scss

There's also card-media__ which makes it even more confusing.

There's also `card-media__` which makes it even more confusing.

Why the change the margin-bottom instead of margin-top? IME always using margin-top/margin-left makes it a lot simpler to add new pieces of ui, because you don't rely on the markup before your new component to provide the spacing.

It also requires removing the margin on :last-child everywhere.

Why the change the `margin-bottom` instead of `margin-top`? IME always using `margin-top`/`margin-left` makes it a lot simpler to add new pieces of ui, because you don't rely on the markup before your new component to provide the spacing. It also requires removing the margin on `:last-child` everywhere.

src/renderer/scss/component/_credit.scss

This should just use something like $--large.

This should just use something like `$--large`.

NetOpWibby (Migrated from github.com) reviewed

2018-11-13 00:18:33 +01:00

src/renderer/component/common/credit-amount.jsx

					
				@ -77,14 +75,11 @@ class CreditAmount extends React.PureComponent<Props> {

				    return (

				      <span

For reference while I was fixing my branch.

NetOpWibby (Migrated from github.com) reviewed

2018-11-13 00:18:49 +01:00

src/renderer/component/dateTime/view.jsx

This was an oversight on my part.

NetOpWibby (Migrated from github.com) reviewed

2018-11-13 00:20:24 +01:00

src/renderer/page/file/view.jsx

					
				@ -163,90 +159,85 @@ class FilePage extends React.Component<Props> {

				    return (

				      <Page forContent>

After seeing the changes in master, it became apparent that --file-page wasn't the best name.

I separated media and card b/c it was quite confusing. I'll have to give them another pass.

After seeing the changes in master, it became apparent that `--file-page` wasn't the best name. I separated `media` and `card` b/c it was quite confusing. I'll have to give them another pass.

NetOpWibby (Migrated from github.com) reviewed

2018-11-13 00:20:54 +01:00

src/renderer/page/fileListDownloaded/view.jsx

Another oversight.

neb-b (Migrated from github.com) reviewed

2018-11-13 00:23:52 +01:00

src/main/createWindow.js

I think we can keep this at 950 for now. Once lbryweb gets going we can finish the expandable menu stuff.

neb-b (Migrated from github.com) reviewed

2018-11-13 00:25:46 +01:00

src/renderer/component/app/view.jsx

What does this do?

NetOpWibby (Migrated from github.com) reviewed

2018-11-13 00:29:26 +01:00

src/renderer/scss/component/_card.scss

Copying from Bootstrap's docs:

Avoid margin-top. Vertical margins can collapse, yielding unexpected results. More importantly though, a single direction of margin is a simpler mental model.

It's consistent with how the rest of the app is styled. Also, the removal of the margin on :last-child is only inside this block. I haven't seen anything to indicate visual issues.

Copying from Bootstrap's docs: > Avoid margin-top. Vertical margins can collapse, yielding unexpected results. More importantly though, **a single direction of margin is a simpler mental model**. It's consistent with how the rest of the app is styled. Also, the removal of the margin on `:last-child` is only inside this block. I haven't seen anything to indicate visual issues.

NetOpWibby (Migrated from github.com) reviewed

2018-11-13 00:34:11 +01:00

src/renderer/component/app/view.jsx

When scrolling on the homepage past the padding, the wunderbar doesn't have a border/divider that separates it from the content.

Comment out this line and scroll on a page in Settings to see what I mean.

When scrolling on the homepage past the padding, the wunderbar doesn't have a border/divider that separates it from the content. [Comment out this line](https://github.com/lbryio/lbry-desktop/blob/newnew/src/renderer/scss/component/_page.scss#L21) and scroll on a page in Settings to see what I mean.

neb-b (Migrated from github.com) reviewed

2018-11-13 00:36:52 +01:00

src/renderer/scss/component/_card.scss

👍 I'm fine moving to margin-bottom

👍 I'm fine moving to `margin-bottom`

neb-b (Migrated from github.com) reviewed

2018-11-13 00:42:04 +01:00

src/renderer/component/app/view.jsx

Why not make that the default behavior? It seems like it's just adding a class to give it the same look as before scrolling (with the border bottom). Why can't we always keep the z-index on the header?

NetOpWibby (Migrated from github.com) reviewed

2018-11-13 22:21:19 +01:00

src/renderer/component/app/view.jsx

It doesn't look good.

NetOpWibby (Migrated from github.com) reviewed

2018-11-14 21:14:58 +01:00

src/renderer/component/fileDetails/view.jsx

Doesn't make sense here. This is inside a media__info block.

Doesn't make sense here. This is inside a `media__info` block.

neb-b (Migrated from github.com) reviewed

2018-11-15 17:24:35 +01:00

src/renderer/component/app/view.jsx

In what case doesn't it look good? If we move the z-index down to this line https://github.com/lbryio/lbry-desktop/blob/newnew/src/renderer/scss/component/_page.scss#L25, we never get to a state where the border-bottom disappears

In what case doesn't it look good? If we move the z-index down to this line https://github.com/lbryio/lbry-desktop/blob/newnew/src/renderer/scss/component/_page.scss#L25, we never get to a state where the border-bottom disappears <img width="400" alt="screen shot 2018-11-15 at 11 23 25 am" src="https://user-images.githubusercontent.com/16882830/48566490-efdad780-e8c8-11e8-95d8-ce7b091816fd.png">

neb-b commented

2018-11-15 18:42:55 +01:00

(Migrated from github.com)

I can't seem to reply in the original thread but for https://github.com/lbryio/lbry-desktop/pull/2102#discussion-diff-232837723L43

The element__element1__element2 naming is not following BEM naming
https://en.bem.info/methodology/quick-start/#nesting-1

I can't seem to reply in the original thread but for https://github.com/lbryio/lbry-desktop/pull/2102#discussion-diff-232837723L43 The `element__element1__element2` naming is not following BEM naming https://en.bem.info/methodology/quick-start/#nesting-1

NetOpWibby (Migrated from github.com) reviewed

2018-11-15 18:43:03 +01:00

src/renderer/component/app/view.jsx

When you're on the first view of the app, the border doesn't look good.

NetOpWibby commented

2018-11-15 18:44:04 +01:00

(Migrated from github.com)

I realized that a while after I made the comment but wasn't able to find it.

neb-b (Migrated from github.com) requested changes

2018-11-15 23:18:51 +01:00

neb-b (Migrated from github.com) left a comment

Did another pass but I didn't make it all the way through. It's looking better but I think the complex style rules are causing inconsistency. I'm in favor of using a .media class to style files, but currently, that styling is intertwined across the app. It should be self-contained.

The other large issue is with spacing on content/buttons across the app. That seems to be due to the nested styles/:not uses applying margin-top or margin-bottom based on where it is in the markup. I think a much simpler solution would be to use one and always stick with that, that will make things more consistent.

Also after debugging a bit on your branch I very strongly believe we should avoid the &--xxx syntax. There were times I searched for a class name and didn't see all of the results because some were hidden due to that shorthand.

Did another pass but I didn't make it all the way through. It's looking better but I think the complex style rules are causing inconsistency. I'm in favor of using a `.media` class to style files, but currently, that styling is intertwined across the app. It should be self-contained. The other large issue is with spacing on content/buttons across the app. That seems to be due to the nested styles/`:not` uses applying margin-top or margin-bottom based on where it is in the markup. I think a much simpler solution would be to use one and always stick with that, that will make things more consistent. Also after debugging a bit on your branch I very strongly believe we should avoid the `&--xxx` syntax. There were times I searched for a class name and didn't see all of the results because some were hidden due to that shorthand.

src/renderer/component/button/view.jsx

If we are changing the class name we should probably change the prop too. I do like constrict more.

If we are changing the class name we should probably change the prop too. I do like `constrict` more.

src/renderer/component/categoryList/view.jsx

					
				@ -223,0 +228,4 @@

				                    button="alt"

				                    showSnackBarOnSubscribe

				                    uri={`lbry://${categoryLink}`}

				                  />

Since .next and .previous have the same styles, we can combine them. Probably something even more generic than .btn--arrow. Maybe they don't need a custom class at all? The only thing it does is add opacity: 0.3; when it's disabled. Should that be a generic styling on all buttons?

The styling should also be removed from _media.scss and into button (if we need it all)

Since `.next` and `.previous` have the same styles, we can combine them. Probably something even more generic than `.btn--arrow`. Maybe they don't need a custom class at all? The only thing it does is add `opacity: 0.3;` when it's disabled. Should that be a generic styling on all buttons? The styling should also be removed from `_media.scss` and into button (if we need it all)

src/renderer/component/channelTile/view.jsx

					
				@ -75,7 +77,7 @@ class ChannelTile extends React.PureComponent<Props> {

				            </React.Fragment>

Maybe this should be media--tile? I think it's really confusing to make this have the "tile" style with the media/media--small class. Also if you have a FileTile with no set size, it will now break, since the display: flex property is only assigned to media--small and ``mediainside of.search`.

This also brings up another issue. The styling for media is in several files. The flex property is set on media--small here and set for media--large inside of the search styles here.

The media--tile (or whatever it's called) should be a top level style that sets display flex and anything else that is common to tiles, and search shouldn't know about media styles.

Maybe this should be `media--tile`? I think it's really confusing to make this have the "tile" style with the `media`/`media--small` class. Also if you have a `FileTile` with no set size, it will now break, since the `display: flex` property is only assigned to `media--small` and ``media` inside of `.search`. This also brings up another issue. The styling for `media` is in several files. The `flex` property is set on `media--small` [here](https://github.com/lbryio/lbry-desktop/pull/2102/files#diff-6787a2ed41b9d19ffd3e8f8419c4806dR55) and set for `media--large` inside of the search styles [here](https://github.com/lbryio/lbry-desktop/pull/2102/files#diff-542ca9e38423f8ec474fc2f07f39044fR3). The `media--tile` (or whatever it's called) should be a top level style that sets display flex and anything else that is common to tiles, and search shouldn't know about media styles.

src/renderer/component/channelTile/view.jsx

I think media__actions and card__actions can be combined. I see some of the modifier classes are the exact same, and some aren't being used.

I think `media__actions` and `card__actions` can be combined. I see some of the modifier classes are the exact same, and some aren't being used.

src/renderer/component/common/credit-amount.jsx

					
				@ -77,14 +75,11 @@ class CreditAmount extends React.PureComponent<Props> {

				    return (

I think we should keep the badge styling. With these changes, it might be confusing if we want to use the badge style on something that isn't media, like the lbc amount in the rewards card on the wallet page). That card isn't related tomedia

It's ok if we have more than one "type" of class on a component.

I think we should keep the `badge` styling. With these changes, it might be confusing if we want to use the badge style on something that isn't media, like the lbc amount in the rewards card on the wallet page). That card isn't related to`media` It's ok if we have more than one "type" of class on a component.

src/renderer/component/common/lbc-symbol.jsx

Are we sure we want to switch to this? Currently, this will be the only reference to this icon in the entire LBRY ecosystem. I think we should wait until we are sure this is the icon we will use.

src/renderer/component/fileCard/view.jsx

We shouldn't have classes like .small (although it doesn't look it is doing anything).

If we have media--tile, maybe this could be media--card?

We shouldn't have classes like `.small` (although it doesn't look it is doing anything). If we have `media--tile`, maybe this could be `media--card`?

src/renderer/component/fileListSearch/view.jsx

This should be search__results-title

This should be `search__results-title`

src/renderer/component/fileTile/view.jsx

					
				@ -77,6 +78,7 @@ class FileTile extends React.PureComponent<Props> {

				      claim,

				      metadata,

				      isResolvingUri,

				      isSearchResult,

This .small class isn't doing anything. Currently the placeholder is an exact representation of the actual FileTile, with all the same classes. That makes styling it very simple.

This `.small` class isn't doing anything. Currently the placeholder is an exact representation of the actual `FileTile`, with all the same classes. That makes styling it very simple.

src/renderer/component/fileTile/view.jsx

Same comments as I had for ChannelTile. This component should proabaly be combined with that one since they are basically the same, but that is out of scope for this PR. They should have the same style since they can be used interchangeably in the ui.

Same comments as I had for `ChannelTile`. This component should proabaly be combined with that one since they are basically the same, but that is out of scope for this PR. They should have the same style since they can be used interchangeably in the ui.

src/renderer/component/fileViewer/internal/play-button.jsx

These classes should reference that they are specifically tied to the btn class.

These classes should reference that they are specifically tied to the `btn` class.

src/renderer/component/fileViewer/view.jsx

This isn't doing anything.

src/renderer/component/header/view.jsx

This should be header__navigation-item

This should be `header__navigation-item`

And the other header classes should follow the same pattern.

src/renderer/component/inviteNew/view.jsx

					
				@ -52,0 +50,4 @@

				        <div className="card__content">

				          <div className="card__actions">

				            <Submit label="Invite" disabled={isPending} />

I don't think I'm a fan of this. Before it was super simple to add a list (or single) button. It was one card__actions class that had a padding-top. I think this could work, but it just takes more effort. A good example is on the wallet page. I see three buttons, with three different top/bottom spacings

I don't think I'm a fan of this. Before it was super simple to add a list (or single) button. It was one `card__actions` class that had a `padding-top`. I think this could work, but it just takes more effort. A good example is on the wallet page. I see three buttons, with three different top/bottom spacings <img width="600" alt="screen shot 2018-11-15 at 5 05 02 pm" src="https://user-images.githubusercontent.com/16882830/48584752-a1453180-e8f8-11e8-836e-079d3c6ef798.png">

src/renderer/component/recommendedContent/view.jsx

There should only be one modifier per class.

src/renderer/scss/component/_media.scss

This button styling should be able to be removed. We can also remove the cursor: default in media__message

This button styling should be able to be removed. We can also remove the `cursor: default` in `media__message`

src/renderer/scss/component/_placeholder.scss

Why remove the animation from this file? If the animation is on the placeholder, I think it makes sense to keep it here.

src/renderer/scss/component/_search.scss

This isn't the correct aspect ratio.

The media styles shouldn't change just because they are on the search page. They should be contained to the _media.scss file.

The media styles shouldn't change just because they are on the search page. They should be contained to the `_media.scss` file.

neb-b (Migrated from github.com) reviewed

2018-11-15 23:26:44 +01:00

src/renderer/component/app/view.jsx

I still see the border appear at a random time depending on my scrolling speed. I don't see any issues with keeping the border always.

NetOpWibby (Migrated from github.com) reviewed

2018-11-16 00:01:21 +01:00

src/renderer/scss/component/_placeholder.scss

It made sense to put all the animation in one file.

NetOpWibby (Migrated from github.com) reviewed

2018-11-16 00:03:55 +01:00

src/renderer/component/channelTile/view.jsx

					
				@ -75,7 +77,7 @@ class ChannelTile extends React.PureComponent<Props> {

				            </React.Fragment>

Agreed, it's a bit of a mess. media-tile sounds like a good name. So we'll have card, media, and media-tile.

Agreed, it's a bit of a mess. `media-tile` sounds like a good name. So we'll have `card`, `media`, and `media-tile`.

neb-b (Migrated from github.com) reviewed

2018-11-16 00:07:02 +01:00

src/renderer/scss/component/_placeholder.scss

Ah I see. I missed that.

neb-b (Migrated from github.com) reviewed

2018-11-16 00:08:24 +01:00

src/renderer/component/channelTile/view.jsx

					
				@ -75,7 +77,7 @@ class ChannelTile extends React.PureComponent<Props> {

				            </React.Fragment>

Maybe media-card? Or if the base class is media the tile style would be a modifier so media--tile

Maybe `media-card`? Or if the base class is `media` the tile style would be a modifier so `media--tile`

neb-b (Migrated from github.com) reviewed

2018-11-16 00:09:25 +01:00

src/renderer/component/fileDetails/view.jsx

The docs say to avoid double element naming https://en.bem.info/methodology/quick-start/#nesting-1

NetOpWibby (Migrated from github.com) reviewed

2018-11-19 19:21:23 +01:00

src/renderer/scss/component/_search.scss

The media on the search page are not displayed the same as everywhere else. My previous design kept the design uniform but that was unwanted.

neb-b (Migrated from github.com) reviewed

2018-11-19 19:25:28 +01:00

src/renderer/scss/component/_search.scss

Other places may not display the exact same data, but they still use the same underlying component.

The component should have the same style, regardless of where it is used in the app. Properties passed in such as size or displayDescription may add additional info to it, or scale up/down, but the style should be the same.

Other places may not display the exact same data, but they still use the same underlying component. The component should have the same style, regardless of where it is used in the app. Properties passed in such as `size` or `displayDescription` may add additional info to it, or scale up/down, but the style should be the same.

kauffj (Migrated from github.com) reviewed

2018-11-19 20:06:57 +01:00

src/renderer/component/common/lbc-symbol.jsx

Yes, do not switch this yet.

NetOpWibby (Migrated from github.com) reviewed

2018-11-19 20:33:47 +01:00

src/renderer/component/common/lbc-symbol.jsx

Should I also remove it from here? I added it to spur conversation and hopefully get a definitive answer. I'll revert the change.

<img width="142" alt="screen shot 2018-11-19 at 1 32 41 pm" src="https://user-images.githubusercontent.com/1288356/48730330-a27ba480-ebff-11e8-839e-fc5b8e51ab70.png"> Should I also remove it from here? I added it to spur conversation and hopefully get a definitive answer. I'll revert the change.

NetOpWibby (Migrated from github.com) reviewed

2018-11-19 23:05:21 +01:00

src/renderer/component/inviteNew/view.jsx

					
				@ -52,0 +50,4 @@

				        <div className="card__content">

				          <div className="card__actions">

				            <Submit label="Invite" disabled={isPending} />

I just inspected the code and the buttons themselves don't have any spacing. The container they are in also don't have spacing unless they are inside another class. I could remove .form-row--padded from the address in Receive Credits (which has top/bottom spacing) to reduce the vertical spacing.

Other than that, I don't see anything wrong with taking the top-down approach to making layouts (pushing down rather than up).

I just inspected the code and the buttons themselves don't have any spacing. The container they are in also don't have spacing unless they are inside another class. I could remove `.form-row--padded` from the address in **Receive Credits** (which has top/bottom spacing) to reduce the vertical spacing. Other than that, I don't see anything wrong with taking the top-down approach to making layouts (pushing down rather than up).

neb-b (Migrated from github.com) requested changes

2018-11-20 23:10:46 +01:00

neb-b (Migrated from github.com) left a comment

Added some more comments. Additionally we lost the main--contained class so all the pages with cards now extend full width. This looks pretty weird when you have a very large display.

Also checkout the page when you go to help then submit a bug report, that style is messed up.

Added some more comments. Additionally we lost the `main--contained` class so all the pages with cards now extend full width. This looks pretty weird when you have a very large display. Also checkout the page when you go to `help` then `submit a bug report`, that style is messed up.

src/renderer/component/common/credit-amount.jsx

					
				@ -77,14 +75,11 @@ class CreditAmount extends React.PureComponent<Props> {

				    return (

I still think this should be called badge. The rewards card has nothing to do with media

I still think this should be called `badge`. The rewards card has nothing to do with media

src/renderer/component/fileCard/view.jsx

If we are removing this we should remove it from the defaultProps on line 34

If we are removing this we should remove it from the `defaultProps` on line 34

src/renderer/component/header/view.jsx

With the new icons that were added we now have two ways of adding icons to buttons. We should consolidate this into one.

src/renderer/component/inviteNew/view.jsx

					
				@ -52,0 +50,4 @@

				        <div className="card__content">

				          <div className="card__actions">

				            <Submit label="Invite" disabled={isPending} />

My comment was more aimed at the inconsistency of the button spacings. It seems like this still isn't fixed. The Claim Rewards button has a large amount of space above, and small amount of space below. The Get New Address button has a large amount of space on top and bottom. And the Full History button has a small amount of space on top and bottom.

My comment was more aimed at the inconsistency of the button spacings. It seems like this still isn't fixed. The `Claim Rewards` button has a large amount of space above, and small amount of space below. The `Get New Address` button has a large amount of space on top and bottom. And the `Full History` button has a small amount of space on top and bottom.

The container they are in also don't have spacing unless they are inside another class.

This seems like it could cause issues. Why not just give the base level class the spacing instead of only adding it if it's inside of another class?

> The container they are in also don't have spacing unless they are inside another class. This seems like it could cause issues. Why not just give the base level class the spacing instead of only adding it if it's inside of another class?

src/renderer/scss/component/_channel.scss

					
				@ -0,0 +1,25 @@

				.channel-info {

				  align-items: center;

				  display: flex;

				  justify-content: space-between;

Before the channel info actions (subscribe/share) were moved below the title because on large 4k screens, they would be waaaay to the right, and easy to miss. I think we should keep them below the title.

src/renderer/scss/component/_credit.scss

This shouldn't be called file-page. If the class is only removing color, maybe there should two classes credit-amount--no-color and credit-amount--large?

This shouldn't be called `file-page`. If the class is only removing color, maybe there should two classes `credit-amount--no-color` and `credit-amount--large`?

src/renderer/scss/component/_navigation.scss

We should use the longform &.class--modifier

We should use the longform `&.class--modifier`

src/renderer/scss/component/_notice.scss

This should use .notice--error

This should use `.notice--error`

src/renderer/scss/component/_search.scss

I think all margins should be based off $spacing-vertical, instead of having a bunch of different margins in many different places

I think all margins should be based off `$spacing-vertical`, instead of having a bunch of different margins in many different places

src/renderer/scss/component/_snack-bar.scss

					
				@ -14,2 +9,2 @@

				  opacity: 0.95;

				  padding: 14px 20px 10px 20px;

				  padding: var(--spacing-vertical-small) var(--spacing-vertical-large) var(--spacing-vertical-small)

				    var(--spacing-vertical-medium); // 0.5rem 1.25rem 0.5rem 1rem;

Should we have a global border-radius variable?

neb-b commented

2018-11-20 23:12:22 +01:00

(Migrated from github.com)

Forgot a couple things

the subscription button still moves down a bit when the heart icon is added/removed.
Disabled buttons still have :hover style changes
Should the links on the file page have any hover style? It's kinda hard to tell they are buttons.

Forgot a couple things - the subscription button still moves down a bit when the `heart` icon is added/removed. - Disabled buttons still have `:hover` style changes - Should the links on the file page have any hover style? It's kinda hard to tell they are buttons.

NetOpWibby (Migrated from github.com) reviewed

2018-11-26 19:04:24 +01:00

src/renderer/component/header/view.jsx

What is being consolidated?

NetOpWibby (Migrated from github.com) reviewed

2018-11-26 19:07:03 +01:00

src/renderer/component/fileCard/view.jsx

I removed this b/c the linter was complaining. Thought I got everything.

NetOpWibby (Migrated from github.com) reviewed

2018-11-26 19:07:54 +01:00

src/renderer/component/fileCard/view.jsx

Should static defaultProps still be there? Just empty, or removed entirely?

Should `static defaultProps` still be there? Just empty, or removed entirely?

neb-b (Migrated from github.com) reviewed

2018-11-26 19:10:36 +01:00

src/renderer/component/header/view.jsx

We should either always use a class and add the icons through css with background-image like you do here, or stick with using props and explicitly render an icon like we are currently doing where we use react-feather

We should either always use a class and add the icons through css with `background-image` like you do [here](https://github.com/lbryio/lbry-desktop/pull/2102/files#diff-88b5fbd05c1ee805da8402786b5cc714R58), or stick with using props and explicitly render an icon like [we are currently doing](https://github.com/lbryio/lbry-desktop/blob/master/src/renderer/component/common/icon.jsx#L3) where we use `react-feather`

neb-b (Migrated from github.com) reviewed

2018-11-26 19:11:18 +01:00

src/renderer/component/fileCard/view.jsx

If there aren't any default props, it can be removed entirely

NetOpWibby (Migrated from github.com) reviewed

2018-11-26 19:11:24 +01:00

src/renderer/scss/component/_search.scss

I don't think everything having the same margin makes sense.

neb-b (Migrated from github.com) reviewed

2018-11-26 19:15:48 +01:00

src/renderer/scss/component/_search.scss

They don't need to have the same margins, but some factor of the base level marign. If we just had small, medium, and large margins, where small is 1/3 the base margin, medium is 1/2 and large is the base margin (these are just random numbers), it would greatly help with consistency.

If everything is on it's own, every component could have different margins if future changes aren't tracked carefully after this is merged.

They don't need to have the same margins, but some factor of the base level marign. If we just had small, medium, and large margins, where small is 1/3 the base margin, medium is 1/2 and large is the base margin (these are just random numbers), it would greatly help with consistency. If everything is on it's own, every component could have different margins if future changes aren't tracked carefully after this is merged.

NetOpWibby (Migrated from github.com) reviewed

2018-11-26 19:21:07 +01:00

src/renderer/scss/component/_credit.scss

This isn't being used anymore.

NetOpWibby (Migrated from github.com) reviewed

2018-11-26 19:29:40 +01:00

src/renderer/scss/component/_channel.scss

					
				@ -0,0 +1,25 @@

				.channel-info {

				  align-items: center;

				  display: flex;

				  justify-content: space-between;

The Publish link in the URL bar is also to the right. Should that be removed too?

NetOpWibby (Migrated from github.com) reviewed

2018-11-26 19:36:18 +01:00

src/renderer/component/inviteNew/view.jsx

					
				@ -52,0 +50,4 @@

				        <div className="card__content">

				          <div className="card__actions">

				            <Submit label="Invite" disabled={isPending} />

These buttons are also in different contexts.

Claim Rewards is below a header. Everything below a header has the same spacing.
Get New Address is below a copyable input. That spacing ensures you don't click on something unintentionally.
Full History is below a table and pertains to that table. Why would it have large spacing above it? The spacing below it is consistent with every single card.

If you want all the buttons to have "consistent spacing", they should all be in the same contexts. But they're not. Because it doesn't make sense.

These buttons are also in different contexts. - `Claim Rewards` is below a header. Everything below a header has the same spacing. - `Get New Address` is below a copyable input. That spacing ensures you don't click on something unintentionally. - `Full History` is below a table and _pertains to that table_. Why would it have large spacing above it? The spacing below it is consistent with every single card. If you want all the buttons to have "consistent spacing", they should all be in the same contexts. But they're not. Because it doesn't make sense.

NetOpWibby (Migrated from github.com) reviewed

2018-11-26 19:37:20 +01:00

src/renderer/component/header/view.jsx

What happens when we want to use an icon that doesn't exist in react-feather?

What happens when we want to use an icon that doesn't exist in `react-feather`?

NetOpWibby commented

2018-11-26 20:40:17 +01:00

(Migrated from github.com)

~~'media--search-result': isSearchResult is not working for channelTile. Not sure what needs to be done there.~~

~`'media--search-result': isSearchResult` is not working for `channelTile`. Not sure what needs to be done there.~

NetOpWibby commented

2018-11-26 20:42:09 +01:00

(Migrated from github.com)

~~What is messed up about this style?~~

~What is messed up about this style?~

neb-b commented

2018-11-26 21:07:01 +01:00

(Migrated from github.com)

'media--search-result': isSearchResult is not working for channelTile. Not sure what needs to be done there.

You have to pass in that value when you use it <ChannelTile ... isSearchResult />

What is messed up about this style?

General inconsistency compared to the rest of the app. There is no margin above the textarea/submit button.

>'media--search-result': isSearchResult is not working for channelTile. Not sure what needs to be done there. You have to pass in that value when you use it `<ChannelTile ... isSearchResult />` >What is messed up about this style? General inconsistency compared to the rest of the app. There is no margin above the textarea/submit button.

NetOpWibby (Migrated from github.com) reviewed

2018-11-26 22:40:18 +01:00

src/renderer/component/channelTile/view.jsx

The only modifier class that's the same is --between.

The only modifier class that's the same is `--between`.

neb-b (Migrated from github.com) requested changes

2018-11-29 07:53:51 +01:00

neb-b (Migrated from github.com) left a comment

Re-opened a comment about the naming /re media/media-card/media-tile

Re-opened a comment about the naming /re `media`/`media-card`/`media-tile`

src/renderer/component/channelTile/view.jsx

					
				@ -75,7 +77,7 @@ class ChannelTile extends React.PureComponent<Props> {

				            </React.Fragment>

This still exists

src/renderer/component/expandable/view.jsx

					
				@ -48,6 +48,7 @@ export default class Expandable extends PureComponent<Props, State> {

				        </div>

				        <Button

				          button="link"

				          className="expandable__button"

Does a regular link not work? If not this should be a button class btn--expandable, but it should just use a regular link button

Does a regular `link` not work? If not this should be a button class `btn--expandable`, but it should just use a regular link button

src/renderer/component/fileCard/view.jsx

As re-opened above, this should be something like media-card

As re-opened above, this should be something like `media-card`

src/renderer/component/fileViewer/internal/play-button.jsx

This should still use the icon prop.

icon={ICONS.PLAY}

This should still use the `icon` prop. `icon={ICONS.PLAY}`

src/renderer/component/header/view.jsx

In the past, we have just chosen a different icon. This comment was more about still using the Icon component. I think it's ok if we use different icons there (it should be avoided, but fine if absolutely necessary), but we definitely shouldn't be adding icons in two different ways.

Everything should go through the Icon component and we shouldn't be adding icons by using a class (.play). I have some ideas but maybe we should discuss over slack. We could check to see if we have a custom icon, if one isn't there, then fallback to react-feather.

In the past, we have just chosen a different icon. This comment was more about still using the `Icon` component. I think it's ok if we use different icons there (it should be avoided, but fine if absolutely necessary), but we definitely shouldn't be adding icons in two different ways. Everything should go through the `Icon` component and we shouldn't be adding icons by using a class (`.play`). I have some ideas but maybe we should discuss over slack. We could check to see if we have a custom icon, if one isn't there, then fallback to react-feather.

src/renderer/component/header/view.jsx

These should use the icon prop

These should use the `icon` prop

src/renderer/component/inviteNew/view.jsx

					
				@ -52,0 +50,4 @@

				        <div className="card__content">

				          <div className="card__actions">

				            <Submit label="Invite" disabled={isPending} />

I understand not having a large space on the Full History button. I think that is fine.

For Get New Address and Claim Rewards, they are both directly above help text, but have two different spacings. Looks like one is p.help and the other is div.help. Might just be a typo.

I understand not having a large space on the `Full History` button. I think that is fine. For `Get New Address` and `Claim Rewards`, they are both directly above help text, but have two different spacings. Looks like one is `p.help` and the other is `div.help`. Might just be a typo.

neb-b commented

2018-11-29 07:58:17 +01:00

(Migrated from github.com)

The button under the textarea should have the same spacing as it would under a table (like transaction history) right? I think those are the same context.

Also the spacing seems really weird when there are cards with subtitles and without. The margin-bottom on the card-header class is causing inconsistent spacing. The space between the textarea and the subtitle on the top card is ~36 pixes, and the spacing between the top link and the title on the bottom card is ~42 pixels.

<img width="400" alt="screen shot 2018-11-29 at 1 54 03 am" src="https://user-images.githubusercontent.com/16882830/49204407-c8304880-f379-11e8-9807-008b98354d98.png"> The button under the textarea should have the same spacing as it would under a table (like transaction history) right? I think those are the same context. Also the spacing seems really weird when there are cards with subtitles and without. The margin-bottom on the `card-header` class is causing inconsistent spacing. The space between the textarea and the subtitle on the top card is ~36 pixes, and the spacing between the top link and the title on the bottom card is ~42 pixels.

NetOpWibby (Migrated from github.com) reviewed

2018-12-05 19:20:43 +01:00

src/renderer/component/header/view.jsx