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

Radio Group margin is inconsistent with previous versions #1139

Closed
mramaiya opened this issue Jan 19, 2023 · 6 comments
Closed

Radio Group margin is inconsistent with previous versions #1139

mramaiya opened this issue Jan 19, 2023 · 6 comments
Assignees
Labels
bug Things that aren't working right in the library.

Comments

@mramaiya
Copy link

mramaiya commented Jan 19, 2023

Unknown if this is a bug or was intended, but previously sl-radio group had no margin, whereas now it has a left-margin at least.

The underlying css previously came from:

.radio-group:not(.radio-group--has-fieldset) {
    border: none;
    padding: 0px;
    margin: 0px;
    min-width: 0px;
}

but that is no longer being applied to the form-control part in sl-radio-group.

This shows that the fieldset adds a margin of 4px (at least on chrome)

<script type="module" src="https://cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.88/dist/shoelace.js"></script>

<sl-radio-group label="Select an option" help-text="Select an option that makes you proud." name="a" value="1">
  <sl-radio-button value="1">Option 1</sl-radio-button>
  <sl-radio-button value="2">Option 2</sl-radio-button>
  <sl-radio-button value="3">Option 3</sl-radio-button>
</sl-radio-group>
@mramaiya mramaiya added the bug Things that aren't working right in the library. label Jan 19, 2023
@CetinSert
Copy link

CetinSert commented Jan 19, 2023

sl.rt.ht/1139 ← easy link
sl.rt.ht/1139? ← append ? for the playground of this issue


Cannot reproduce this in .88 vs .87 down to .84. Screenshots are from Edge ≈ Chrome.

.88 .87
image image

To quickly switch between versions:

  1. Go to sl.rt.ht/1139?
    Versions will be selected.
  2. Press /

@claviska claviska changed the title sl-radio-group margin inconsistent with previous version Radio Group margin inconsistent with previous version Jan 19, 2023
@claviska claviska changed the title Radio Group margin inconsistent with previous version Radio Group margin is inconsistent with previous versions Jan 19, 2023
@claviska
Copy link
Member

I'm also unable to reproduce this in the latest version.

https://jsfiddle.net/8bqth46c/

There was a somewhat recent change to Radio Group which removed the fieldset attribute and improved its overall appearance. They stemmed from #965 and were implemented in f03b09a.

From what I can tell, everything looks as expected in 2.0.0-beta.88. If you have additional information, feel free to post it and I'll reopen to investigate!

@mramaiya
Copy link
Author

I was comparing against 63 I think.

I do see a left margin of 4px on the fieldset (under sl-radio-group>shadow-root) in the link you gave me. On the prior version we were using, it had a margin of 0px (using the css I posted earlier)

Overall, noit a big deal though.

@claviska
Copy link
Member

Hmm, I think I see what you're seeing. There is a slight gap between the host and the internal <fieldset>.

CleanShot 2023-01-20 at 11 18 58@2x

I'll get this fixed.

claviska added a commit that referenced this issue Jan 20, 2023
@claviska
Copy link
Member

Fixed in 5f9bbdf. This fix will be available in the next release.

@mramaiya
Copy link
Author

awesome! thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Things that aren't working right in the library.
Projects
None yet
Development

No branches or pull requests

3 participants