Make alignment great again #2450

2019-04-28T21:44:30+02:00

neb-b commented

2019-04-28 21:44:30 +02:00

(Migrated from github.com)

For https://github.com/lbryio/lbry-desktop/issues/2412

~~Dependent on https://github.com/lbryio/components/pull/8 and https://github.com/lbryio/components/pull/9~~

Changes

Tried to simplify the header/button styles
- Removed excessive html markup
- Tried to get rid of as many top: {x}px type styles as I could
Brought back the round buttons
- Square buttons are hard to make look polished
- A lot of extra styles are needed to really differentiate buttons from inputs/static markup

For https://github.com/lbryio/lbry-desktop/issues/2412 ~Dependent on https://github.com/lbryio/components/pull/8 and https://github.com/lbryio/components/pull/9~ #### Changes - Tried to simplify the header/button styles - Removed excessive html markup - Tried to get rid of as many `top: {x}px` type styles as I could - Brought back the round buttons - Square buttons are hard to make look polished - A lot of extra styles are needed to really differentiate buttons from inputs/static markup

kauffj (Migrated from github.com) approved these changes 2019-05-02 22:39:45 +02:00

kauffj (Migrated from github.com) left a comment

Mergeable

src/ui/component/button/view.jsx

					
				@ -21,4 +21,3 @@

				  type: string,

				  button: ?string, // primary, secondary, alt, link

				  noPadding: ?boolean, // to remove padding and allow circular buttons

				  iconColor?: string,

kauffj (Migrated from github.com) commented

2019-05-02 22:36:06 +02:00

Nice. We should kill properties that are style only like this whenever possible.

src/ui/scss/component/_header.scss

					
				@ -10,1 +7,4 @@

				  z-index: 2; // Main content uses z-index: 1, this ensures it always scrolls under the header

				  background-color: $lbry-white;

				  border-bottom: 1px solid $lbry-gray-1;

kauffj (Migrated from github.com) commented

2019-05-02 22:39:03 +02:00

Not necessary to adjust in this PR, but it can be good to make your z-index values SASS variables for better clarity.

src/ui/scss/component/_icon.scss

					
				@ -0,0 +2,4 @@

				// Minor adjustments to ensure icons line up vertically

				.icon--Heart {

				  top: -1px;

kauffj (Migrated from github.com) commented

2019-05-02 22:39:28 +02:00

How will this scale when icons are rendered at different sizes?

neb-b (Migrated from github.com) reviewed 2019-05-02 23:45:49 +02:00

src/ui/scss/component/_icon.scss

					
				@ -0,0 +2,4 @@

				// Minor adjustments to ensure icons line up vertically

				.icon--Heart {

				  top: -1px;

neb-b (Migrated from github.com) commented

2019-05-02 23:45:49 +02:00

That's a good point. This won't scale well. I think it will work if we use em. Currently our icons are all locked in at the same size, regardless of font size. Will keep that in mind going forward.

That's a good point. This won't scale well. I think it will work if we use `em`. Currently our icons are all locked in at the same size, regardless of font size. Will keep that in mind going forward.

Sign in to join this conversation.

No reviewers

kauffj