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

[Primitives] BasePrimitive #2842

Merged
merged 19 commits into from
Jun 12, 2024
Merged

[Primitives] BasePrimitive #2842

merged 19 commits into from
Jun 12, 2024

Conversation

KenAJoh
Copy link
Collaborator

@KenAJoh KenAJoh commented Apr 11, 2024

Description

Inspirert av Radix Themes tilbyr de et sett med "base"-props for alle Primitivene sine. BasePrimitive-komponenten er en forsøk på å løse det samme, og vil være en intern komponent som gjenbrukes av de andre.

Hvordan tiltenkt bruk er:

// Box.tsx

BoxProps extends BasePrimtiveProps...

<BasePrimtive {...baseprops}>
    <div boxcomponent-props: ref classname style ... />
</BasePrimtive/>

Siden BasePrimtive er en "Slot"-komponent vil det ikke føre til ekstra html-noder.

Usikker på om denne bør extende alle komponentene, gir f.eks kanskje ikke mening på show/hide og bleed(?)

WIP tiltenkte props denne base-komponenten tilbyr

Alt er oppe i luften her, så alt av innspill eller idèer er supert!

  • padding, padding-inline, padding-block
  • margin, margin-inline, margin-block
  • width, minWidth, maxWidth
  • height, minHeight, maxHeight
  • position: absolute, fixed, sticky etc
  • om position blir med: inset, top, left, right, bottom
  • Ikke 100% på denne: flex-basis, flex-shrink, flex-grow
  • eller denne: grid-column, grid-column-start, grid-column-end, grid-row, grid-row-start, grid-row-end

Copy link

changeset-bot bot commented Apr 11, 2024

⚠️ No Changeset found

Latest commit: 5a5faf8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link
Contributor

github-actions bot commented Apr 11, 2024

Storybook demo

Endringer til review: 2

42561bf69 | 85 komponenter | 160 stories

@KenAJoh KenAJoh marked this pull request as ready for review May 29, 2024 08:40
@KenAJoh KenAJoh changed the title 🚧 BasePrimitive-konsept 🚧 [Primitives] BasePrimitive May 31, 2024
@navikt/core/react/src/layout/base/BasePrimitive.tsx Outdated Show resolved Hide resolved
@navikt/core/react/src/layout/base/BasePrimitive.tsx Outdated Show resolved Hide resolved
@navikt/core/react/src/layout/base/BasePrimitive.tsx Outdated Show resolved Hide resolved
@navikt/core/react/src/layout/base/BasePrimitive.tsx Outdated Show resolved Hide resolved
Comment on lines +84 to +87
/**
* CSS `overflow`
*/
overflow?: ResponsiveProp<"visible" | "hidden" | "clip" | "scroll" | "auto">;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should revisit the JSDOC later. It's a little inconsistent that we write about the object option only on some of the props, when it applies to all of them. Maybe the JSDOC for each prop should be really simple and short, with a link to some place with more details 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds like a good idea, think ResponsiveProp and in general the Primitives concept deserves a documentation-page under /grunnleggende when its implemented and tested. Can link to that documentation then 👍

@KenAJoh KenAJoh merged commit 221f7c5 into main Jun 12, 2024
5 checks passed
@KenAJoh KenAJoh deleted the base-primitive branch June 12, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants