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

Issue #5: Add radio group component. #72

Merged
merged 6 commits into from
Jul 23, 2019
Merged

Conversation

jcfranco
Copy link
Member

@jcfranco jcfranco commented Jul 19, 2019

Related Issue: #5

This adds the RadioGroup and RadioGroupItem components.

API

interface CalciteRadioGroup {
  name: string;
  selectedItem: CalciteRadioGroupItem | null;
  theme: "light" | "dark";

  // events
  calciteRadioGroupChange: EventEmitter;
}

interface CalciteRadioGroupItem {
  checked: boolean;
  value: any;

  // events - internal
  calciteRadioGroupItemChange: EventEmitter;
}
// CSS vars
--calcite-radio-group-color
--calcite-radio-group-color-hover
--calcite-radio-group-color-active
--calcite-radio-group-border-color
--calcite-radio-group-text-color
--calcite-radio-group-text-color-active

Questions

  • Since item components are not meant to be used outside, would it be cleaner to hide the internal event and rename the group's event to calciteRadioGroupChange? Changed to calciteRadioGroupChange.
  • Would selected instead of checked be a better fit since there's no visual checkmark in this design?
  • Can I get some feedback on the public CSS vars? Do we need fewer/more?

Notes

  • Has some placeholder styles for the dark theme. @julio8a Could you tweak?
  • Complies w/ https://www.w3.org/TR/wai-aria-practices-1.1/#radiobutton with the following exceptions:
    • Labeling of <calcite-radio-group> is left to users (via aria-labelledby or <label>). Alternatively, we could introduce <calcite-radio-group-title>, design permitting.
    • No handling of For Radio Group Contained in a Toolbar section.
  • Uses hidden input for form compatibility.
  • Calling focus() on the group component does not work as expected. I'll create a separate issue for this.

@jcfranco jcfranco added this to the 1.0.0 milestone Jul 19, 2019
@jcfranco jcfranco self-assigned this Jul 19, 2019
});
});

describe("WAI-ARIA Roles, States, and Properties", () => {
Copy link
Member

Choose a reason for hiding this comment

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

I like how you've broken up the tests into sections 👌

@paulcpederson
Copy link
Member

@jcfranco if you use npm run build it should generate the markdown docs for your component if you'd like those to be part of this pull request.

@paulcpederson
Copy link
Member

paulcpederson commented Jul 20, 2019

Also, you asked for feedback on css vars: I am not totally sure what our strategy should be. In loader and slider I just gave devs enough to change the main branded color (ie. change blue to green) but didn't open up the various neutrals/grays for customization. My thought is that by far the most common thing most people will want to do is change the active color to their branded color. I don't think we should allow devs to completely override all the colors of a component, but maybe we should?

We could also have a css var that operates library-wide, allowing people to theme these components really easily by setting the one spot color to their branded color and having it cascade all the way through the components.

Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

👍

jcfranco added 2 commits July 22, 2019 13:45
* Add private/protected modifiers.
* Remove unnecessary initialization.
@jcfranco jcfranco requested a review from paulcpederson July 22, 2019 20:52
@paulcpederson
Copy link
Member

@jcfranco I just noticed that the external inputs appear in the tab order, even though they are hidden.

@jcfranco
Copy link
Member Author

@paulcpederson On it!

@jcfranco
Copy link
Member Author

@paulcpederson Fixed. The external inputs were still being rendered in the DOM.

jcfranco added 2 commits July 23, 2019 12:52
* Fix cursor not showing up on item label.
* Tweak colors.
* Rename vars.
@jcfranco
Copy link
Member Author

  • Updated vars and colors (both light and dark)
  • Renamed calciteRadioGroupItemSelect to calciteRadioGroupChange.

@jcfranco jcfranco merged commit 0c0b2b8 into master Jul 23, 2019
@jcfranco jcfranco deleted the jcfranco/5-add-radio-group branch July 23, 2019 20:17
jcfranco added a commit that referenced this pull request May 13, 2024
…e empty with a start/end icon (#9300)

*Related Issue:** #6413

## Summary

This updates `segmented-control-item` to display a centered icon when
specified and the item is empty.

**Note:** this removes using `value` as a fallback label as non-breaking
for the following reasons:

* this behavior is
[intentional](https://github.com/Esri/calcite-design-system/blob/main/packages/calcite-components/src/components/segmented-control-item/segmented-control-item.e2e.ts#L38-L46),
but there is no explicit spec for it in the [original
issue](#5),
[PR](#72) nor
[documentation](https://developers.arcgis.com/calcite-design-system/components/segmented-control/)
* it is inconsistent with how other components expect text to be
provided
* it [breaks if there's any
whitespace](https://codepen.io/jcfranco/pen/XWwWGEy?editors=1000)
* the current behavior will lead to label that might not be
user-friendly in most cases (e.g., casing, localization)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants