Skip to content
This repository has been archived by the owner on Sep 21, 2023. It is now read-only.

fix(AdSettingsCog + SvgIconWrapper): Changed cogwheel and svgIconWrapper type #1375

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AmerSharif
Copy link
Contributor

AdSettingsCog: Changed from div to button because of wcag2 feedback
SvgIconWrapper: Changed from div to span to avoid having a block element inside button.

Please tick a box

  • feature (A new feature)
  • fix (A bug fix)
  • refactor (A code change that neither fixes a bug nor adds a feature)
  • docs (Documentation only changes)
  • performance (A code change that improves performance)
  • style (Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc))
  • build (Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm))
  • ci (Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs))
  • test (Adding missing tests or correcting existing tests)

If you're adding a feature, is it documented in a storybook story?

  • Yes
  • No
  • Not adding a new feature

Are the components you're working on reusable between brands?

  • Yes (Using variables that are defined in the default theme)
  • No (I hard coded some values)
  • Not working on any components

If you're creating markup, did you add proper semantics?

(Did you do a CR and see that there is something that we should check for each PR, that are not on the list, please update this document)

@AmerSharif AmerSharif requested a review from misund August 26, 2019 13:26
Copy link
Contributor

@misund misund left a comment

Choose a reason for hiding this comment

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

The changes in AdSettingsCog are fine. Is there a reason for the wrapper change? The largest difference between a div and a span is the initial display-property.

@@ -1,6 +1,6 @@
import styled from '@emotion/styled';

const SvgIconWrapper = styled('div', {
const SvgIconWrapper = styled('span', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed SvgIconWrapper to avoid putting a div (block element) inside a button element. Best practice i think :S Should i avoid doing it?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants