Just PR for virtualize UI to review #5290
Conversation
ghost
requested review from
|
Deploy preview for carbon-elements failed. Built with commit e4dd544 https://app.netlify.com/sites/carbon-elements/deploys/5e3c48cec7002a000a0a4069 |
|
Deploy preview for carbon-components-react failed. Built with commit e4dd544 https://app.netlify.com/sites/carbon-components-react/deploys/5e3c48ce0ed8930008810329 |
|
Deploy preview for carbon-elements ready! Built with commit 68dfa0e |
|
Deploy preview for carbon-components-react ready! Built with commit 68dfa0e https://deploy-preview-5290--carbon-components-react.netlify.com |
|
Looks good! Not seeing any issues with the Primary button or any of the filled variants, but I'm seeing some issues with the Tertiary button w/ Icon. It seems like when it receives focus, it shifts the icon to the left a bit. Here is an example: Tertiary Primary, for reference |
|
Oh, nice catch 👍 let me investigate it |
|
hi @tw15egan, Tertiary button seems working fine in my dev local. Just only 1 concern about the padding of tooltip will increase 1px after this change. |
|
Hmm, I'm not seeing that issue with the tooltip button...
What browser/os are you using? |
|
macOS Mojave 10.14.6 and Chrome 79.0.3945 |
|
this is related to #5263 right? |
asudoh
requested review from
a team and
laurenmrice
and removed request for
a team
|
when you have a chance could you rebuild so the react link doesn't fail. thanks ! |
|
Just triggered a rebuild, should be ready shortly |
|
hmm weird, this is what i see in my local |
|
|
||
| /// @type Number | ||
| /// @access public | ||
| /// @group button | ||
| $button-outline-width: 1px !default; | ||
| $button-outline-width: 2px !default; |
There was a problem hiding this comment.
I think we just need to keep this at 1px
There was a problem hiding this comment.
I originally read it as 2px on both as well 😃
There was a problem hiding this comment.
We need to keep this at 1px
There was a problem hiding this comment.
yeah i think once that is fixed we should be good^
xxxle0
force-pushed
the
duytran/5263-outline-focus-button-width
branch
from
12d5176 to
8ea9af9
Compare
xxxle0
force-pushed
the
duytran/5263-outline-focus-button-width
branch
from
1a107cb to
a957b1f
Compare
|
hi @tw15egan , @laurenmrice , can you guys review this PR again ? |
|
here is the outline 2px i think it should be 2px. Btw if we only change button-border-width, the last button outline will difference from another buttons |
|
@xxxle0 Then we will need some sort of override on icon only buttons to make sure they are consistent with the rest of the buttons. Design spec is 1px so that is what we will adhere to. |
|
yeah ok i got it. thanks @tw15egan |
|
@xxxle0 do you have any plans to finish up this PR? |
|
Seems like this issue will be bundled in with some larger button changes in #5618, so I'm going to close this out. |
Closes #5290
Just a experimental PR for reviewing UI after change css