search

Couchers-org / couchers

The next-generation couch surfing platform. Free forever. Community‑led. Non‑profit. Modern. Chuck us a star :)
https://couchers.org
MIT License
375 stars 77 forks source link

Custom `Button` component contains invalid element #4167

Open wbruntra opened 2 years ago

wbruntra commented 2 years ago

As discovered by @goskan93, (https://github.com/Couchers-org/couchers/pull/2037) the custom Button component renders a div by default, which is invalid inside a button (https://stackoverflow.com/questions/51755195/is-it-a-valid-markup-to-put-some-tags-inside-button-tag).

So it seems the Button component needs to be redesigned.

lucaslcode commented 2 years ago

My bad. The div in the component can just be changed to a span. It's there to make the text opacity 0 for async actions, so the button doesn't change size.

darrenvong commented 2 years ago

In that case, I don't think we can use the CircularProgress within the button either as it wraps the spinner within a div and I'm not sure there's a way to override the underlying element used for its container...

goskan93 commented 2 years ago

Tbh, I just meant that @wbruntra put an 'a' tag inside 'button' tag, and you noticed this div :) CircularProgress from Mui is made of span and svg,

wbruntra commented 2 years ago

In that case, I don't think we can use the CircularProgress within the button either as it wraps the spinner within a div and I'm not sure there's a way to override the underlying element used for its container...

Looks like in mui v4 it uses a div, but in v5 it uses a span, so perhaps better to just ignore this issue for now? It will eventually fix itself.

goskan93 commented 2 years ago

In that case, I don't think we can use the CircularProgress within the button either as it wraps the spinner within a div and I'm not sure there's a way to override the underlying element used for its container...

Looks like in mui v4 it uses a div, but in v5 it uses a span, so perhaps better to just ignore this issue for now? It will eventually fix itself.

true, I looked at v5 and not v4 xp yeah, I think the CircularProgress can stay as it is for now, and only change this div https://github.com/Couchers-org/couchers/blob/develop/app/web/src/components/Button/Button.tsx#L87