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

fix(RadioGroup): use regular components instead of Label #1070

Merged
merged 4 commits into from
Mar 19, 2019

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Mar 18, 2019

Fixes #1040.


BREAKING CHANGES

  1. Generated markup was changed:

Before

    <div
      role="radio"
      tabindex="-1"
      aria-checked="false"
      aria-disabled="true"
      class="ui-radiogroup__item dd aj"
    >
      <span class="ui-label bt ht et gy hz ii cw ib ic id bf bm hu">
        <span class="ui-icon ek aj bu ak ai ie if ig ce hv ij ik hy" aria-hidden="true"></span>
        <span dir="auto">Prosciutto</span>
      </span>
    </div>

After

    <div
      role="radio"
      tabindex="-1"
      aria-checked="false"
      aria-disabled="true"
      class="ui-radiogroup__item jo bf bt bm jp dd jq ie"
    >
      <span class="ui-icon ek aj bu ak ai ju jv jw ce jr il jy jt" aria-hidden="true"></span>
      <span dir="auto" class="ui-box">Prosciutto</span>
    </div>
  1. label is now shorthand value, see Shorthand Props

@codecov
Copy link

codecov bot commented Mar 18, 2019

Codecov Report

Merging #1070 into master will decrease coverage by <.01%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1070      +/-   ##
==========================================
- Coverage    81.9%   81.89%   -0.01%     
==========================================
  Files         693      693              
  Lines        8448     8447       -1     
  Branches     1233     1163      -70     
==========================================
- Hits         6919     6918       -1     
  Misses       1514     1514              
  Partials       15       15
Impacted Files Coverage Δ
...eams/components/RadioGroup/radioGroupItemStyles.ts 20% <0%> (+1.81%) ⬆️
...s/components/RadioGroup/radioGroupItemVariables.ts 100% <100%> (ø) ⬆️
...react/src/components/RadioGroup/RadioGroupItem.tsx 100% <100%> (ø) ⬆️
packages/react/src/components/Label/Label.tsx 97.05% <0%> (-2.95%) ⬇️

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 27ef22f...a9fce2f. Read the comment docs.

@layershifter layershifter added 🚀 ready for review 🧰 fix Introduces fix for broken behavior. labels Mar 18, 2019
{rtlTextContainer.createFor({ element: label })}
</Label>
</ElementType>
{Box.create(label, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change the type of the label prop to ShorthandValue?

CHANGELOG.md Outdated
@@ -16,6 +16,7 @@
This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]
- Use regular components instead of `Label` in `RadioGroupItem` @layershifter ([#XXX](https://github.com/stardust-ui/react/pull/XXX))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add section above this (Breaking change probably if we introduce changes of the typings)

Copy link
Collaborator

@kolaps33 kolaps33 left a comment

Choose a reason for hiding this comment

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

Tested with readers. And it looks good :) 👍

Copy link
Contributor

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

The code looks good. Be sure to fix the screener regressions before merging.

@layershifter layershifter merged commit abe2d6c into master Mar 19, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/radio-label branch March 19, 2019 12:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants