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

Ability to Specify RadioButton Check Size #4465

Closed
halocline opened this issue Aug 27, 2020 · 11 comments · Fixed by grommet/grommet-theme-hpe#130
Closed

Ability to Specify RadioButton Check Size #4465

halocline opened this issue Aug 27, 2020 · 11 comments · Fixed by grommet/grommet-theme-hpe#130
Assignees
Labels
enhancement A suggestion to add to or change behavior in progress Currently in development

Comments

@halocline
Copy link
Collaborator

Expected Behavior

Would like to be able to specify the check size of a RadioButton in my theme.

Actual Behavior

My design calls for a 16x16 check (r=8), but the default icon is 12x12 (r=6).

Related Issue: grommet/hpe-design-system#851

Proposal for something like radioButton.check.size

URL, screen shot, or Codepen exhibiting the issue

Steps to Reproduce

Your Environment

  • Grommet version: 2.15.0 / Stable
  • Browser Name and version:
  • Operating System and version (desktop or mobile):
@ShimiSun ShimiSun added the enhancement A suggestion to add to or change behavior label Aug 31, 2020
@netochaves
Copy link
Contributor

I open a PR for this #4505

@ShimiSun ShimiSun added the in progress Currently in development label Sep 22, 2020
@ShimiSun
Copy link
Member

ShimiSun commented Oct 6, 2020

Can we try leveraging the existing theme prop theme.radioButton.icons.circle instead of adjusting the radius on the code?

@halocline
Copy link
Collaborator Author

Can we try leveraging the existing theme prop theme.radioButton.icons.circle instead of adjusting the radius on the code?

@ShimiSun Ah, I remember why I abandoned this path initially. Initially, I had wanted to add

icons: {
  circle: () => (
    <Blank color="selected-background">
      <circle cx="12" cy="12" r="8" />
    </Blank>
  ),
}

to the theme file. That worked great in a local theme, but did not translate well to grommet-theme-hpe. When trying to apply to that project, the following changes would need to be made to enable this:

  • import React to the theme file
  • 'react' then must be added as a dependency, most likely a peer dependency in this case.
  • Additionally, we would then need to update our Babel configs, adding @babel/preset-react as a preset, so that Babel knows how to parse index.js as JSX.
  • And then there was something else, that I can't remember.

At the time, this felt like a lot of overhead to get at what we needed to accomplish. Plus, felt that others might like this ability as well.

Thoughts on whether we want to pursue these types of changes on grommet-theme-hpe?

Another approach would be to add a larger circle to grommet-icons, but that seemed like a pretty acute need.

@ShimiSun
Copy link
Member

ShimiSun commented Oct 8, 2020

@halocline why not simply using SVG instead of the Blank react component?

@halocline
Copy link
Collaborator Author

@halocline why not simply using SVG instead of the Blank react component?

We'd still be relying on JSX to define the <svg> in this case as well, correct? If so, I think we run into the same issues.

@ShimiSun
Copy link
Member

@halocline I think we'll be OK adding a peer dependency and use the JSX solution, can you file a PR on grommet-theme-hpe to start the discussion?

@halocline
Copy link
Collaborator Author

@halocline I think we'll be OK adding a peer dependency and use the JSX solution, can you file a PR on grommet-theme-hpe to start the discussion?

👍 Will do.

@halocline
Copy link
Collaborator Author

Closing this. Used the property theme.radioButton.icons.circle to provide a custom-sized SVG.

@netochaves
Copy link
Contributor

Sorry for not providing the necessary support on this. Thanks @halocline, great solution btw,

@halocline
Copy link
Collaborator Author

Sorry for not providing the necessary support on this. Thanks @halocline, great solution btw,

@netochaves, no, thank you! @ShimiSun was able to point out that Grommet already had support for this, but the property had been undocumented. So it made sense to use the functionality which already existed and document it (which it is now). Very much appreciate all of your contributions!

@ShimiSun
Copy link
Member

@netochaves Your PR had definitely helped us move the discussion forward and help us find the best solution, Thanks again!! Regardless, your overall contributions to grommet are highly appreciated!! Looking forward to seeing more of your work ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A suggestion to add to or change behavior in progress Currently in development
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants