Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(Button): remove left and right padding of tertiary button #1860

Merged
merged 7 commits into from
Jan 9, 2023
Merged

fix(Button): remove left and right padding of tertiary button #1860

merged 7 commits into from
Jan 9, 2023

Conversation

tujoworker
Copy link
Member

@tujoworker tujoworker commented Jan 6, 2023 •
edited
Loading

The tertiary button has space to its left and right to ensure space to other elements, much like a text link. But our other buttons do not have a space, so this PR is about to align to the other button variants and remove the space entirely.

The main commit is this one here.

For reference:

Screenshot 2023-01-06 at 10 23 42

Without padding (proposed):

Screenshot 2023-01-06 at 10 23 33

With padding (currently):
Screenshot 2023-01-06 at 10 23 10

@tujoworker tujoworker requested a review from langz January 6, 2023 09:24
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 6, 2023 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cca2b41:

Sandbox Source
eufemia-starter Configuration

@gatsby-cloud
Copy link

gatsby-cloud bot commented Jan 6, 2023 •
edited
Loading

✅ DNB Eufemia Portal deploy preview ready

@langz
Copy link
Contributor

langz commented Jan 6, 2023

Good job :)
I'll help do some testing and reviewing this 🙏

@langz
Copy link
Contributor

langz commented Jan 6, 2023 •
edited
Loading

My 1st observation is the screenshot tests, seems to be some alignment issue and/or spacing issues(see red lines):

Screenshot 2023-01-06 at 10 48 16

@tujoworker
Copy link
Member Author

My 1st observation is the screenshot tests, seems to be some alignment issue and/or spacing issues(see red lines):

Very good observations! Looks like we have to fix these padding's.
That makes me think of it should rather be a v10 fix? I initially thought this should be ok 😰

@langz
Copy link
Contributor

langz commented Jan 6, 2023 •
edited
Loading

My 1st observation is the screenshot tests, seems to be some alignment issue and/or spacing issues(see red lines):

Very good observations! Looks like we have to fix these padding's. That makes me think of it should rather be a v10 fix? I initially thought this should be ok 😰

Hmm, good point 💯
We can and should of course fix our components to look the same as before, like fix the spacing in the breadcrumbs(see image) to make it look as it used to, so then there wouldn't be any "breaking change" for the breadcrumb component.

Screenshot 2023-01-06 at 12 24 42

But most likely the same visual changes as in breadcrumb can/will happen in other applications using the tertiary button as well, and that's perhaps a "breaking change"? 🤔

@tujoworker
Copy link
Member Author

I'm thinking of removing the padding on the first item? What do you think?

Proposed

Screenshot 2023-01-06 at 16 05 23

Current

Screenshot 2023-01-06 at 16 05 42

@tujoworker tujoworker force-pushed the fix/tertiary-button-padding branch 2 times, most recently from e2b29e7 to b9438e2 Compare January 6, 2023 15:19
@langz
Copy link
Contributor

langz commented Jan 6, 2023

I'm thinking of removing the padding on the first item? What do you think?

Proposed

Screenshot 2023-01-06 at 16 05 23

Current

Screenshot 2023-01-06 at 16 05 42

I like it :)
Must be easier for apps using this to align it with other stuff now when there's no left padding/spacing on the first item.

@tujoworker tujoworker changed the base branch from main to v10 January 9, 2023 08:06
@tujoworker tujoworker force-pushed the fix/tertiary-button-padding branch from b9438e2 to ad3d9d0 Compare January 9, 2023 08:15
@tujoworker
Copy link
Member Author

Lets make it a part of v10 then 🥤

@langz
Copy link
Contributor

langz commented Jan 9, 2023 •
edited
Loading

Nice nice, the padding/spacing in the breadcrumb got improved I will say. Seems like the chevron is more "centered" now than previously(was center but more to the right):

image

@tujoworker
chore(Slider): make visual tests more reliable for when Tooltip is re…
f7567f0 
…ndered (#1875)

* chore(Slider): make visual tests more reliable for when Tooltip is rendered

I hope at least so. Se the first commit. the rest is snapshot name changes due to JS to TS file extension change.

* chore: update snapshots
@tujoworker tujoworker force-pushed the fix/tertiary-button-padding branch from 2cebc2e to f7567f0 Compare January 9, 2023 09:32
@tujoworker tujoworker merged commit a90bd2e into v10 Jan 9, 2023
@tujoworker tujoworker deleted the fix/tertiary-button-padding branch January 9, 2023 10:34
tujoworker added a commit that referenced this pull request Jan 26, 2023
@tujoworker
fix(Breadcrumb): ensure focus state is not partially hidden
29dc02c 
With PR #1860 we got this issue:

By adding a visual test, simulating the focus state, we should avoid this in the future.

Its fixed by removing the `overflow: hidden` – its not used actually, because our animation `HeightAnimation` will add&remove it during the animation anyway.
@tujoworker tujoworker mentioned this pull request Jan 26, 2023
Merged
tujoworker added a commit that referenced this pull request Jan 26, 2023
@tujoworker
fix(Breadcrumb): ensure focus state is not partially hidden (#1949)
954d570 
With PR #1860 we got this issue:

By adding a visual test, simulating the focus state, we should avoid this in the future.

Its fixed by removing the `overflow: hidden` – its not used actually, because our animation `HeightAnimation` will add&remove it during the animation anyway.
tujoworker pushed a commit that referenced this pull request Jan 29, 2023
@semantic-release-bot
chore(release): 9.46.2 [skip ci]
84fe476 
## [9.46.2](v9.46.1...v9.46.2) (2023-01-29)

### Bug Fixes

* **Breadcrumb:** ensure focus state is not partially hidden ([#1949](#1949)) ([954d570](954d570)), closes [#1860](#1860)
* **Table:** fix fist border on rowSpan={2} is used in first column ([#1956](#1956)) ([48ff543](48ff543))
* **Table:** fix sticky table when in iFrame ([#1954](#1954)) ([ac4f254](ac4f254))
joakbjerk pushed a commit that referenced this pull request Mar 27, 2023
@tujoworker @joakbjerk
fix(Breadcrumb): ensure focus state is not partially hidden (#1949)
80350dd 
With PR #1860 we got this issue:

By adding a visual test, simulating the focus state, we should avoid this in the future.

Its fixed by removing the `overflow: hidden` – its not used actually, because our animation `HeightAnimation` will add&remove it during the animation anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@langz langz langz approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tujoworker @langz