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.

Sign up for GitHub

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

Jump to bottom

fix(ToggleGroup): align with new css and api principles #2424

Merged
merged 7 commits into from
Sep 17, 2024
Merged

fix(ToggleGroup): align with new css and api principles #2424

merged 7 commits into from
Sep 17, 2024

Conversation

eirikbacker
Copy link
Contributor

@eirikbacker eirikbacker commented Sep 16, 2024 •
edited
Loading

Part of #2295 and #2221

@eirikbacker eirikbacker self-assigned this Sep 16, 2024
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Sep 16, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 5628e86

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 GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 16, 2024 •
edited
Loading

Preview deployments for this pull request:

Storybook 17. Sep 2024 - 09:18 (Norwegian time)

Theme 17. Sep 2024 - 09:19 (Norwegian time)

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 16, 2024 •
edited
Loading

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 63.66% 4588 / 7206
🔵 Statements 63.66% 4588 / 7206
🔵 Functions 86.24% 163 / 189
🔵 Branches 75.57% 591 / 782
File CoverageNo changed files found.
Generated in workflow #386

@github-actions GitHub Actions
Copy link
Contributor

Preview deployments for this pull request:

Theme 16. Sep 2024 - 15:01 (Norwegian time)

@eirikbacker eirikbacker mentioned this pull request Sep 16, 2024
Closed
@github-actions GitHub Actions
Copy link
Contributor

Preview deployments for this pull request:

Theme 16. Sep 2024 - 15:02 (Norwegian time)

@github-actions GitHub Actions
Copy link
Contributor

Preview deployments for this pull request:

Theme 16. Sep 2024 - 15:03 (Norwegian time)

@github-actions GitHub Actions
Copy link
Contributor

Preview deployments for this pull request:

Theme 16. Sep 2024 - 15:31 (Norwegian time)

@github-actions GitHub Actions
Copy link
Contributor

Preview deployments for this pull request:

Theme 17. Sep 2024 - 07:48 (Norwegian time)

Barsnes
Barsnes reviewed Sep 17, 2024
View reviewed changes
packages/react/src/components/ToggleGroup/ToggleGroup.tsx
*/
export const ToggleGroupRoot = forwardRef<HTMLDivElement, ToggleGroupProps>(
(
export const ToggleGroup = forwardRef<HTMLDivElement, ToggleGroupProps>(
Copy link
Member

Choose a reason for hiding this comment

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

This has a context and is required for ToggleGroup to work, do we want to go without the Context suffix because we render a div 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest yes; .Context is only used when not rendering a element, ref;
#2221 (comment)

For the end user, it is not important wether or not a context is used under the hood, but it is important wether or not a DOM element is rendered :)

Copy link
Member

Choose a reason for hiding this comment

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

As long as we have consistent rules. We should probably make this support asChild as well

Copy link
Member

Choose a reason for hiding this comment

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

That however means we have to move the hidden input outside of the wrapping div 🤔

Copy link
Contributor Author

@eirikbacker eirikbacker Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we need to support asChild on the wrapper? ☺️ I see the point for Buttons inside, but changing the DOM node for the wrapper I am unsure of the usage

Copy link
Member

Choose a reason for hiding this comment

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

It's just for consistency, but we can look at this later on.

@eirikbacker eirikbacker merged commit c2b78ed into next Sep 17, 2024
6 checks passed
@github-actions github-actions bot mentioned this pull request Sep 17, 2024
Merged
mimarz pushed a commit that referenced this pull request Sep 20, 2024
@github-actions
chore: new release candidate (next) (#2434)
95ba93c 
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to next, this PR will
be updated.

⚠️⚠️⚠️⚠️⚠️⚠️

`next` is currently in **pre mode** so this branch has prereleases
rather than normal releases. If you want to exit prereleases, run
`changeset pre exit` on `next`.

⚠️⚠️⚠️⚠️⚠️⚠️

# Releases
## @digdir/designsystemet-css@1.0.0-next.34

### Patch Changes

- Textarea: Use `field-sizing: content`
([#2463](#2463))

- ErrorSummary: Rename ErrorSummary.Root to ErrorSummary
([#2437](#2437))

-   Tabs: ([#2448](#2448))

    -   Renames `Tabs.Root` to `Tabs`
    -   Renames `Tabs.Content` to `Tabs.Panel`

- Rename classes from `ds-error-message*` to `ds-validation-message*`
([#2473](#2473))

- Modal: css changes
([#2418](#2418))

- DropdownMenu:
([#2432](#2432))

    -   Rename from `DropdownMenu` to `Dropdown`
    -   Change API and structure
    -   Rename `.Root` to `.Context`
    -   Rename `.Content` to `Dropdown`

- Tabs: css changes
([#2431](#2431))

- ToggleGroup: Rename ToggleGroup.Root to ToggleGroup
([#2424](#2424))

- Badge: Only use single DOM element for rendering
([#2422](#2422))

- Skeleton: Replace Skeleton.Text, Skeleton.Circle and
Skeleton.Rectangle with <Skeleton variant="">
([#2435](#2435))

- Breadcrumbs: Rename `Breadcrumbs.Root` to `Breadcrumbs` and remove
`Breadcrumbs.Nav`
([#2428](#2428))

- HelpText:
([#2438](#2438))

    -   Use Popover API
    -   Remove `portal` prop
    -   Render icon with pseudo element and require aria-label

- Fieldset: Style using css attributes
([#2447](#2447))

## @digdir/designsystemet-react@1.0.0-next.34

### Patch Changes

- Correctly mark dependencies as external. This ensures Accordion works
when consumers have enabled tree-shaking.
([#2479](#2479))

- Button: Remove `type` when `asChild={true}`
([#2472](#2472))

- ErrorSummary: Rename ErrorSummary.Root to ErrorSummary
([#2437](#2437))

-   Tabs: ([#2448](#2448))

    -   Renames `Tabs.Root` to `Tabs`
    -   Renames `Tabs.Content` to `Tabs.Panel`

- Modal: css changes
([#2418](#2418))

- Rename `ErrorMessage` to `ValidationMessage`
([#2473](#2473))

- DropdownMenu:
([#2432](#2432))

    -   Rename from `DropdownMenu` to `Dropdown`
    -   Change API and structure
    -   Rename `.Root` to `.Context`
    -   Rename `.Content` to `Dropdown`

- Tabs: css changes
([#2431](#2431))

- ToggleGroup: Rename ToggleGroup.Root to ToggleGroup
([#2424](#2424))

- Badge: Only use single DOM element for rendering
([#2422](#2422))

- Skeleton: Replace Skeleton.Text, Skeleton.Circle and
Skeleton.Rectangle with <Skeleton variant="">
([#2435](#2435))

- Breadcrumbs: Rename `Breadcrumbs.Root` to `Breadcrumbs` and remove
`Breadcrumbs.Nav`
([#2428](#2428))

- HelpText:
([#2438](#2438))

    -   Use Popover API
    -   Remove `portal` prop
    -   Render icon with pseudo element and require aria-label

- Fieldset: Style using css attributes
([#2447](#2447))

## @digdir/designsystemet@1.0.0-next.34



## @digdir/designsystemet-theme@1.0.0-next.34

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Barsnes Barsnes Barsnes approved these changes

@mimarz mimarz Awaiting requested review from mimarz mimarz is a code owner

Assignees

@eirikbacker eirikbacker

Labels
None yet
Projects
Team Design System
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eirikbacker @Barsnes