feat(Popover): add utility class for inline text

[Barsnes]

resolves #2781

[changeset-bot]

🦋 Changeset detected

Latest commit: ae07913

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@digdir/designsystemet-css	Patch
@digdir/designsystemet-react	Patch
@digdir/designsystemet	Patch
@digdir/designsystemet-theme	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

Preview deployments for this pull request:

Storybook - 19. Dec 2024 - 09:44

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	55.2%	3198 / 5793
🔵	Statements	55.2%	3198 / 5793
🔵	Functions	86.6%	181 / 209
🔵	Branches	76.96%	518 / 673

File Coverage

File	Stmts	Branches	Functions	Lines	Uncovered Lines
Changed Files
packages/react/src/components/Popover/PopoverTrigger.tsx	85%	25%	100%	85%	29-31

Generated in workflow #1351 for commit ae07913 by the Vitest Coverage Report Action

[eirikbacker]

Burde vi legge på type="button" i eksempel så folk ikke glemmer å endre på dette? :)

[Barsnes]

Tenker du at klassen kun targeter type="button"? Det er kanskje ikkje så dumt 🤔

[eirikbacker]

Nei :) Bare at button automatisk er type="submit" om man ikke setter type="button", så det kan fort bli accidental submit om de har en popup-tekstforklaring inni et skjema og glemmer type="button" ☺️

[Barsnes]

right sikkert lurt, kanskje klassen også skulle ha sjekka det - slik at me kan være meir sikre på at dei bruker det rett 🤔

[mimarz]

Maybe this could be data attribute (data-inline) to match the rest of our css where we use that for our modifiers?

[Barsnes]

I think it would be nice if this specifies that it is for popover 🤔

[eirikbacker]

Godt poeng! hva med:

[data-popover="inline"] {
  ...
}

[mimarz]

How about we add a inline prop to our Popover.Trigger to toggle data-inline?

[Barsnes]

How about we add a inline prop to our Popover.Trigger to toggle data-inline?

I'm not sure how well this plays with asChild. We would need to look something like this:

const Component = asChild ? Slot : (inline ? Button : 'button');

return <Component ref={ref} popovertarget={popoverId} {...rest} />;

Which looks weird to me

Since we use all: unset, we run into problems with other classes being used on the same element as well, so it's safer for us to make the user have to create a new element.

I think it is better to ask our users to asChild this onto a plain button.

This is an image of what happens when I put the class directly on the trigger.

[eirikbacker]

How about we add a inline prop to our Popover.Trigger to toggle data-inline?

I'm not sure how well this plays with asChild. We would need to look something like this:

const Component = asChild ? Slot : (inline ? Button : 'button');

return <Component ref={ref} popovertarget={popoverId} {...rest} />;

Which looks weird to me

Since we use all: unset, we run into problems with other classes being used on the same element as well, so it's safer for us to make the user have to create a new element.

Hi! I disagree - it is easier to add a attribute than making the user add children and remembering correct button type etc.
all: unset is not the issue here, it is the specificity of :not([hidden]) in button.css, which actually should be corrected since this is a tweak of the default display, meaning it should be at the same specificity as user agent styles anyway.

If you change:

  &:not([hidden]) {
    display: flex;
  }

in button.css, to:

  /* Using :where as this should be as easy to overwrite as user agent styles */
  &:where(:not([hidden])) {
    display: flex;
  }

it works smoothly :)

[Barsnes]

How about we add a inline prop to our Popover.Trigger to toggle data-inline?

I'm not sure how well this plays with asChild. We would need to look something like this:

const Component = asChild ? Slot : (inline ? Button : 'button');

return <Component ref={ref} popovertarget={popoverId} {...rest} />;

Which looks weird to me
Since we use all: unset, we run into problems with other classes being used on the same element as well, so it's safer for us to make the user have to create a new element.

Hi! I disagree - it is easier to add a attribute than making the user add children and remembering correct button type etc. all: unset is not the issue here, it is the specificity of :not([hidden]) in button.css, which actually should be corrected since this is a tweak of the default display, meaning it should be at the same specificity as user agent styles anyway.

If you change:

  &:not([hidden]) {
    display: flex;
  }

in button.css, to:

  /* Using :where as this should be as easy to overwrite as user agent styles */
  &:where(:not([hidden])) {
    display: flex;
  }

it works smoothly :)

Sure, but we still get hover and active styles..

If we do the proposed solution above, with changing the rendered element, this is not an issue, though - but then your proposed fix is not an issue either.

[mimarz]

LGTM 👍