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

User can see material description #73

Merged

Conversation

Adamik10
Copy link
Contributor

@Adamik10 Adamik10 commented Dec 10, 2024

Link to issue

https://reload.atlassian.net/browse/DDFBRA-169

Description

This PR adds:

  • description for material on the material page
  • CoverPicture stories
  • SlideSelect stories
  • Badge stories
  • Adds InfoBox.tsx + InfoBoxItem.tsx for rendering the work description box
  • various smaller fixes

Screenshot of the result

image
image

Additional comments or questions

There's a bug when redirecting to the search using tag buttons (inside InfoBoxItem.tsx) where it doesn't really update the search. It has something to do with x-state so I'll need Mikkel to help.

- This accessibility issue is now handled inside the <Badge/> comp directly now
- We have a story for both mobile and desktop for each component.
…, WorkPageLayout, CoverPicture, and InfoBox components for improved organization and clarity
Copy link
Contributor

@ThomasGross ThomasGross left a comment

Choose a reason for hiding this comment

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

Good stuff. I have some comments. Lets talk about them 😊

.storybook/preview.tsx Outdated Show resolved Hide resolved
components/pages/searchPageLayout/SearchPageLayout.tsx Outdated Show resolved Hide resolved
components/pages/workPageLayout/WorkPageLayout.tsx Outdated Show resolved Hide resolved
components/shared/coverPicture/CoverPicture.stories.tsx Outdated Show resolved Hide resolved
components/shared/infoBox/InfoBox.tsx Outdated Show resolved Hide resolved
components/shared/infoBox/InfoBox.tsx Outdated Show resolved Hide resolved
components/shared/infoBox/InfoBoxItem.tsx Outdated Show resolved Hide resolved
components/shared/infoBox/InfoBoxItem.tsx Outdated Show resolved Hide resolved
styles/globals.css Outdated Show resolved Hide resolved
Comment on lines +351 to +353
function ({ addVariant }: { addVariant: (name: string, generator: string) => void }) {
addVariant("not-first", "&:not(:first-child)")
},
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this code enable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead use space-y-7 on the parent? I like when spacing are defined one place the parent instead of applying it on each child. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the other way around - we need the margin-top to be there on every <hr> that's not the first <hr>

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me show you what I mean tomorrow :)

components/shared/button/Button.tsx Show resolved Hide resolved
components/shared/infoBox/InfoBox.tsx Outdated Show resolved Hide resolved
Comment on lines +351 to +353
function ({ addVariant }: { addVariant: (name: string, generator: string) => void }) {
addVariant("not-first", "&:not(:first-child)")
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead use space-y-7 on the parent? I like when spacing are defined one place the parent instead of applying it on each child. :)

components/shared/infoBox/InfoBoxItem.tsx Outdated Show resolved Hide resolved
@Adamik10 Adamik10 requested a review from ThomasGross December 16, 2024 15:06
@spaceo spaceo self-requested a review December 17, 2024 09:29
Copy link
Contributor

@spaceo spaceo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Adamik10 Adamik10 merged commit e4d852d into main Dec 17, 2024
9 checks passed
@Adamik10 Adamik10 deleted the DDFBRA-169-bruger-skal-kunne-se-en-beskrivelse-af-materiale branch December 17, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants