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

Add new banner component #651

Merged
merged 6 commits into from
Jul 9, 2024
Merged

Add new banner component #651

merged 6 commits into from
Jul 9, 2024

Conversation

kasperbirch1
Copy link
Contributor

Link to issue

https://reload.atlassian.net/browse/DDFFORM-540

Description

This pull request adds a new banner component that contains a required link and title. It can be used with a description and a background image to enhance visual appeal.

The title field is a ReactNode where the (underline) tag has a custom underlined SVG placed beneath it.

Screenshot of the result

@kasperbirch1 kasperbirch1 force-pushed the DDFFORM-540-bannerparagraph branch 2 times, most recently from 9b943b1 to e77f7c7 Compare June 1, 2024 12:36
kasperbirch1 added a commit to danskernesdigitalebibliotek/dpl-cms that referenced this pull request Jun 1, 2024
kasperbirch1 added a commit to danskernesdigitalebibliotek/dpl-cms that referenced this pull request Jun 1, 2024
@kasperbirch1 kasperbirch1 force-pushed the DDFFORM-540-bannerparagraph branch from e77f7c7 to 2eac008 Compare June 2, 2024 07:39
@kasperbirch1 kasperbirch1 force-pushed the DDFFORM-540-bannerparagraph branch from 2eac008 to 8017f32 Compare June 3, 2024 10:44
kasperbirch1 added a commit to danskernesdigitalebibliotek/dpl-cms that referenced this pull request Jun 3, 2024
@kasperbirch1 kasperbirch1 force-pushed the DDFFORM-540-bannerparagraph branch 2 times, most recently from 89a404b to f12b323 Compare June 4, 2024 07:48
kasperbirch1 added a commit to danskernesdigitalebibliotek/dpl-cms that referenced this pull request Jun 4, 2024
@LasseStaus LasseStaus assigned LasseStaus and unassigned kasperg, rasben and LasseStaus Jun 6, 2024
Copy link
Contributor

@LasseStaus LasseStaus 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 suggestions you can consider.

I think it could be considered if a link is required. Maybe ddf wants the component to also be used without a link. This is potentially out-of-scope here since they didn't specify it, but i think its' worth asking if we haven't.

I'm unsure about the implementation of the image. By doing this, we use everything related to media-container, which has figures, fallbacks etc which might not be necessary.

I would suggest you consider setting the image as a backgrundImage directly inline instead, and simplify the html structure. This is also being done here. Maybe @rasben could chime in what he thinks about that.

I'd suggest you link to related PR's also for convenience 🚀

Good stuff with the underline!

src/styles/scss/tools/mixins.tools.scss Outdated Show resolved Hide resolved
src/stories/Library/banner/banner.scss Outdated Show resolved Hide resolved
src/stories/Library/banner/banner.scss Outdated Show resolved Hide resolved
@LasseStaus LasseStaus assigned kasperbirch1 and unassigned LasseStaus Jun 6, 2024
kasperbirch1 added a commit to danskernesdigitalebibliotek/dpl-cms that referenced this pull request Jun 13, 2024
kasperbirch1 added a commit to danskernesdigitalebibliotek/dpl-cms that referenced this pull request Jun 13, 2024
kasperbirch1 added a commit to danskernesdigitalebibliotek/dpl-cms that referenced this pull request Jun 13, 2024
@kasperbirch1 kasperbirch1 requested a review from LasseStaus June 13, 2024 15:19
@LasseStaus LasseStaus force-pushed the DDFFORM-540-bannerparagraph branch 3 times, most recently from a42d091 to bb9a2e2 Compare June 19, 2024 09:42
@LasseStaus LasseStaus assigned kasperg and unassigned LasseStaus Jun 19, 2024
@LasseStaus LasseStaus requested a review from kasperg June 19, 2024 10:18
Copy link
Contributor

@kasperg kasperg left a comment

Choose a reason for hiding this comment

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

👍 I think this is nice. I have some questions to be considered but it may be too late to do much about them at this point.

There seems to be some edge cases where the component does not appear as well. I do not think that is a major problem though.

 Banner - With Image Only Description ⋅ Storybook 2024-06-20 09-00-24

src/styles/scss/tools/mixins.tools.scss Show resolved Hide resolved
src/stories/Library/banner/Banner.stories.tsx Outdated Show resolved Hide resolved
src/stories/Library/banner/Banner.tsx Outdated Show resolved Hide resolved
src/stories/Library/banner/Banner.tsx Outdated Show resolved Hide resolved
@kasperg kasperg assigned LasseStaus and unassigned kasperg Jun 20, 2024
@LasseStaus LasseStaus force-pushed the DDFFORM-540-bannerparagraph branch from 7b7773b to 55ef7b6 Compare June 21, 2024 06:50
@LasseStaus LasseStaus force-pushed the DDFFORM-540-bannerparagraph branch from 55ef7b6 to 00e28b5 Compare July 2, 2024 08:54
kasperbirch1 and others added 6 commits July 2, 2024 11:02
This component contains a required link and title. It can be used with a description and a background image to enhance visual appeal.

The title field is a `ReactNode` where the `<u>` (underlined) tag has a custom underline SVG placed beneath it.
Replaced the fixed `margin-left` pixels in `banner-content` with a new layout wrapper `banner__spacing` for dynamic spacing.
Rename mixin to be more generic

Enhance responsiveness of component

simplify grid stacking

DDFFORM-540
MediaContainer / image credited is not being correctly used here.
To avoid redundant markup we use backgroundImage inline instead.

DDFFORM-540
@LasseStaus LasseStaus force-pushed the DDFFORM-540-bannerparagraph branch from 00e28b5 to 9a9e7c2 Compare July 2, 2024 09:02
@LasseStaus LasseStaus merged commit 57ab427 into develop Jul 9, 2024
8 checks passed
@LasseStaus LasseStaus deleted the DDFFORM-540-bannerparagraph branch July 9, 2024 07:39
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