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.

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

fix: toolbar button not having any margin #2196

Merged
merged 2 commits into from Mar 22, 2021

Conversation

itmilos
Copy link
Contributor

@itmilos itmilos commented Mar 12, 2021

Related Issue

Closes #1155

Description

Added margins for buttons in toolbar overflow

Screenshots

NOTE: If you've made any style changes, please provide appropriate screenshots (before and after) to help reviewers.

To see examples of which screenshots to include, go to Screenshot Examples.

Before:

image

After:

image

Please check whether the PR fulfills the following requirements

  1. The output matches the design specs
  • All values are in rem
  • [na] Text elements follow the truncation rules
  • [na] hover state of the element follow design spec
  • [na] focus state of the element follow design spec
  • [na] active state of the element follow design spec
  • [na] selected state of the element follow design spec
  • [na] selected hover state of the element follow design spec
  • [na] pressed state of the element follow design spec
  • [na] Responsiveness rules - the component has modifier classes for all breakpoints
  • [na] Includes Compact/Cosy/Tablet design
  • [na] RTL support
  1. The code follows fundamental-styles code standards and style
  • [na] only one top level fd-* class is used in the file
  • [na] BEM naming convention is used
  • [na] Mixins are used for repeatable code (fd-rtl, fd-ellipsis, fd-flex, fd-selected, fd-focus, ect.)
  • [na] A11y support - keyboard support, screenreader support, proper ARIA attributes, etc.
  • [na] fd-reset() mixin is applied to all elements
  • [na] Variables are used, if some value is used more than twice.
  • [na] Checked if current components can be reused, instead of having new code.
  1. Testing
  • tested Storybook examples with "CSS Resources" normalize option
  • tested Storybook examples with "CSS Resources" unnormalize option
  • Verified all styles in IE11
  • [na] Updated tests
  • last commit message should have [ci visual] so it can trigger chromatic visual regression (e.g. test: run chromatic visual regression [ci visual])
  1. Documentation
  • [na] Storybook documentation has been created/updated
  • [na] Breaking Changes wiki has been updated in case of breaking changes.

@itmilos itmilos added this to the Sprint 57 - Edinburgh milestone Mar 12, 2021
@netlify
Copy link

netlify bot commented Mar 12, 2021

Deploy preview for fundamental-styles ready!

Built with commit 963c277

https://deploy-preview-2196--fundamental-styles.netlify.app

@itmilos itmilos changed the title Fix/1155 toolbar button not having any margin fix: toolbar button not having any margin Mar 12, 2021
src/toolbar.scss Outdated
@@ -163,6 +163,16 @@ $title-toolbar-height: 2.75rem;
margin: 0.375rem 0.1875rem;
width: auto;
background-color: var(--sapToolbar_SeparatorColor);
&+ .#{$fd-namespace}-button {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

including fd-button inside fd-toolbar is violating the self-contained rules. If inside your component (in this case fd-toolbar) you are overwriting the properties of another component (fd-button in this case) you can't target directly the element like you did, you need a modifier class.
Basically you will need something like: fd-toolbar__button where you will modify the specific property of fd-button. But the problem here is that the visual specs do not specify spacing for the buttons in the overflow menu. The button is supposed to have "touchable area" that is bigger than the dimensions of the button and this touchable area is playing the role of "margin" around the button

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the specs:
Screen Shot 2021-03-12 at 3 26 30 PM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@InnaAtanasova I think it may be safe to assume the same spacing as buttons in the action sheet menu?

Copy link
Contributor Author

@itmilos itmilos Mar 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

including fd-button inside fd-toolbar is violating the self-contained rules. If inside your component (in this case fd-toolbar) you are overwriting the properties of another component (fd-button in this case) you can't target directly the element like you did, you need a modifier class.

we can apply the same spacing for all elements in the menu, that way we should remove some of classes like

.#{$block}__overflow__label {
&.#{$fd-namespace}-label {
margin: 0.375rem 0;

@InnaAtanasova InnaAtanasova self-requested a review March 18, 2021 14:57
@InnaAtanasova
Copy link
Contributor

@itmilos you prob need to update the storyshot, the builds are failing

@itmilos itmilos force-pushed the fix/1155-toolbar-button-not-having-any-margin branch from 3b9af1d to 963c277 Compare March 22, 2021 17:57
@itmilos itmilos merged commit e968b63 into main Mar 22, 2021
@itmilos itmilos deleted the fix/1155-toolbar-button-not-having-any-margin branch March 22, 2021 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

toolbar button not having any margin
4 participants