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

DDFFORM 6 linkskomponent liste #347

Merged
merged 13 commits into from
Dec 12, 2023
Merged

DDFFORM 6 linkskomponent liste #347

merged 13 commits into from
Dec 12, 2023

Conversation

LasseStaus
Copy link
Contributor

@LasseStaus LasseStaus commented Nov 27, 2023 •
edited
Loading

  • Added internalLink & Searchbooks SVG
  • Fixed typo in URL for figma link to IconCollection
  • Added LinkWithIcon with stories & types.
  • Added CSS for linkWithIcon & linkIcon
  • Add temporary container(size) mixin
  • Added dist.zip to gitignore

Link to issue

Jira ticket

CMS PR

Description

This PR adds a Link component, which can be used for display either an internal link, external link, File download or link to a search result.

  • Added new SVGS
  • Add the link component
  • Fixed typo in figma link for iconCollection.
  • Added temporary container mixin. Will be changed later when we agree on a general structure
  • Added dist.zip to .gitignore (See questions)

Screenshot of the result

image

Additional comments or questions

Consider wether dist.zip in .gitignore has any other unintentional implications.
I just wanted to not unnecesarily see this in git when using the new dapple cms+react setup.

I don't have specifics, but since this is my first design system component, feel free to just comment on anything.

This could be

  • General JSX structure.
  • Specifics to storybook.
  • Specifics to the CSS.
  • Specifics to icon handling
  • If you in general observe something / know something that would be beneficial for me to know going forward developing on this project, then please comment with that.

@LasseStaus LasseStaus changed the base branch from develop to release/2023-49-0 November 27, 2023 12:52
@LasseStaus LasseStaus force-pushed the DDFFORM-6-linkskomponent-liste branch 2 times, most recently from 2ad32bc to 5929caa Compare December 1, 2023 10:34
@LasseStaus LasseStaus mentioned this pull request Dec 1, 2023
Merged
@LasseStaus LasseStaus force-pushed the DDFFORM-6-linkskomponent-liste branch from 5929caa to 375a8de Compare December 1, 2023 14:00
@LasseStaus LasseStaus marked this pull request as ready for review December 1, 2023 14:17
kasperg
kasperg requested changes Dec 3, 2023
View reviewed changes
Copy link
Contributor

@kasperg kasperg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👍 Nice start. Some good stuff here and some things you need to look into.

There is some different things going on here:

  1. The CSS needs to be adjusted to our BEM structure. If you need to read up on BEM we have a few good links for reference in Zulip.
  2. The CSS needs to be adjusted accoridng to our decisions regarding more BEM and less utility classes.
  3. I appreciate you doing a little bit of housekeeping as a part of this PR while keeping it in separate commits.

.gitignore Show resolved Hide resolved
base.scss Outdated Show resolved Hide resolved
src/stories/Library/Icons/icon-collection/IconCollection.stories.tsx Show resolved Hide resolved
src/stories/Library/link-with-icon/LinkWithIcon.tsx Outdated Show resolved Hide resolved
src/stories/Library/link-with-icon/link-with-icon.scss Outdated Show resolved Hide resolved
src/stories/Library/link-with-icon/LinkWithIcon.tsx Outdated Show resolved Hide resolved
src/stories/Library/link-with-icon/LinkWithIcon.tsx Show resolved Hide resolved
src/stories/Library/link-with-icon/LinkWithIcon.stories.tsx Outdated Show resolved Hide resolved
src/stories/Library/link-with-icon/LinkTypes.ts Outdated Show resolved Hide resolved
src/stories/Library/link-with-icon/LinkWithIcon.stories.tsx Show resolved Hide resolved
@kasperg kasperg changed the base branch from release/2023-49-0 to release/2023-51-0 December 5, 2023 12:42
@LasseStaus LasseStaus requested a review from kasperg December 6, 2023 10:42
@LasseStaus LasseStaus force-pushed the DDFFORM-6-linkskomponent-liste branch from ddc65c1 to c8dad19 Compare December 6, 2023 10:45
kasperg
kasperg approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@kasperg kasperg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👍 Very nice update.

I especially appreciate you taking the time to improve the documentation.

This should be good to merge once the linting issues and merge conflicts have been resolved.

Please also remember to approve the changes in Chromatic if you are satisfied with them. I took a look and as I see it they look good.

public/icons/README.md Outdated Show resolved Hide resolved
public/icons/README.md Show resolved Hide resolved
@LasseStaus LasseStaus force-pushed the DDFFORM-6-linkskomponent-liste branch from 5fb1383 to bce6f1c Compare December 8, 2023 14:10
@LasseStaus
Added internalLink & Searchbooks SVG
9d54f40 
DDFFORM-6
@LasseStaus
Fixed typo in URL for figma link to IconCollection
d9022b8 
 DDFFORM-6
@LasseStaus
Added LinkWithIcon LinkIcon with stories & types.
0ec199f 
DDFFORM-6
@LasseStaus
Added CSS for linkWithIcon & linkIcon
a53da75 
DDFFORM-6
@LasseStaus
Add temporary container(size) mixin
1373802
@LasseStaus
Added dist.zip to gitignore
2288fc5 
This folder was being generated on each CSS change, in order to reflect changes in the CMS when using 'dapple' setup. I guess it doesn't hurt adding it here.
@LasseStaus
Moved scss import to appropriate line.
38bdb4b 
DDFFORM-6
@LasseStaus
Removed <div> from linkList decorators.
59c1a8f 
Added addtional link for /advanced-search for clarity.

DDFFORM-6
@LasseStaus
Refactor linkWIthIcon & css to be inline with BEM
9eb23d9 
DDFFORM-6
@LasseStaus
Refactor types to be singular instead of plural
d52b24e 
DDFFORM-6
@LasseStaus
Updated class naming for icons in public/ folder.
de68306 
Some icons that were used for animations were imported as react
components, and have been modified with styling used for the animations.
This styling was not updated in the public/ folder, which is the one being
used in the CMS, and therefore the styling was not consistent.

I've changed the namings of the icons to be consistent with what the react
components are using and added a readme .

Comment on the contents of the readme.md file.
@LasseStaus
Use grid over flex. Removed class from arrowSVG.
1665acf 
Since we're using "source" to use inline SVG's in twig for the cms, we
can't add classes directly to the svg.
Because of this i changed the layout of the link icon to use grid,
instead of flexbox, and removed the class on the svg.
@LasseStaus LasseStaus force-pushed the DDFFORM-6-linkskomponent-liste branch from bce6f1c to fed2c40 Compare December 8, 2023 14:18
@LasseStaus
Move icon guideline documention to /docs folder.
116fae4 
Also added reference in icons folder.

DDFFORM-6
@LasseStaus LasseStaus force-pushed the DDFFORM-6-linkskomponent-liste branch from fed2c40 to 116fae4 Compare December 8, 2023 15:08
@LasseStaus LasseStaus merged commit 70c3042 into release/2023-51-0 Dec 12, 2023
9 checks passed
@LasseStaus LasseStaus deleted the DDFFORM-6-linkskomponent-liste branch December 12, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kasperg kasperg kasperg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LasseStaus @kasperg @rasben @kasperbirch1