Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(styles): removed redundant margins from components and examples #945

Merged
merged 5 commits into from
Feb 26, 2019

Conversation

bmdalex
Copy link
Collaborator

@bmdalex bmdalex commented Feb 22, 2019

fix(styles): removed redundant margins from components and examples

Description

This PR is pretty straightforward. It:

  • removes the marginRight styles from the following components:
    • Button
    • Icon
    • Dialog (as consequence, as it was using 2 Button components next to each other in the implementation)
  • fixes all the examples grouping these components by using Flex to achieve the necessary spacing previously provided by marginRight

Screenshots:

These screenshot outline how spacing is achieved in Button examples where 2 Button components are placed next to each other

Before (with marginRight):

screenshot 2019-02-22 at 10 25 31

After (with gap="gap.small"):

screenshot 2019-02-22 at 10 24 47

@codecov
Copy link

codecov bot commented Feb 22, 2019

Codecov Report

Merging #945 into master will increase coverage by 0.18%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #945      +/-   ##
==========================================
+ Coverage   80.78%   80.96%   +0.18%     
==========================================
  Files         664      664              
  Lines        8493     8496       +3     
  Branches     1495     1495              
==========================================
+ Hits         6861     6879      +18     
+ Misses       1618     1602      -16     
- Partials       14       15       +1
Impacted Files Coverage Δ
.../src/themes/teams/components/Icon/iconVariables.ts 100% <ø> (ø) ⬆️
...rc/themes/teams/components/Input/inputVariables.ts 100% <ø> (ø) ⬆️
.../src/themes/teams/components/Flex/flexVariables.ts 100% <ø> (ø) ⬆️
packages/react/src/components/Flex/Flex.tsx 94.11% <ø> (+44.11%) ⬆️
...act/src/themes/teams/components/Icon/iconStyles.ts 22.95% <ø> (ø) ⬆️
...src/themes/teams/components/Button/buttonStyles.ts 3.7% <ø> (ø) ⬆️
...es/teams/components/Attachment/attachmentStyles.ts 25% <0%> (ø) ⬆️
...teams/components/Attachment/attachmentVariables.ts 100% <100%> (ø) ⬆️
packages/react/src/components/Dialog/Dialog.tsx 32.3% <100%> (+1.05%) ⬆️
...ckages/react/src/components/Button/ButtonGroup.tsx 97.29% <100%> (+0.07%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b8644d...340c303. Read the comment docs.

},
}),
)}
<Flex gap="gap.smaller">
Copy link
Collaborator Author

@bmdalex bmdalex Feb 22, 2019

Choose a reason for hiding this comment

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

@kuzhelov this means introducing an extra DOM node, any suggestions on how to avoid this?
I can think about:

  • adding a marginRight variable and override it for Button when needed
  • or specifying
defaultProps: {
  as: Flex,
}

// ...

renderComponent() {
  // ...
  return <ElementType gap="gap.smaller" {...unhandledProps} className={classes.root} ... />
}

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I would expect the second approach being used here as, essentially, this is what we would like to achieve - i.e. to use Flex as a container.

However, this will introduce one subtle problem (that, depending on the use case, might become not that subtle to spot:). Suppose that client wants to achieve the following

  • use as wrapper (this is the place where example seems to be contrived, but I am pretty sure that there might be more appealing scenario)
  • preserve layout aspects

Note that this use case won't be covered even with the second approach suggested - however, we might use its core idea being a bit extended. To cover mentioned use case I would suggest to use the following implementation strategy:

renderComponent({ ElementType, classes, unhandledProps, ... }) {
  ...
  return <Flex as={ElementType} gap="gap.smaller" {...unhandledProps} className={classes.root} ... />
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good, applied Flex as={ElementType} gap="gap.smaller", thanks

Copy link
Contributor

@kuzhelov kuzhelov left a comment

Choose a reason for hiding this comment

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

just couple of small changes I would suggest to introduce

@bmdalex bmdalex merged commit 8519c74 into master Feb 26, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/styles-remove-margins branch February 26, 2019 12:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants