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

AB# 294 Feature - card component #101

Merged
merged 10 commits into from
Mar 22, 2021
Merged

AB# 294 Feature - card component #101

merged 10 commits into from
Mar 22, 2021

Conversation

mmexvr
Copy link
Contributor

@mmexvr mmexvr commented Mar 22, 2021 •
edited
Loading

Describe pull-request
Describe what the pull-request is about

Added card-webcomponent

  • Basic with headlines
  • Supporting text
  • clickable option
  • Divider
  • Actions links/buttons
  • Image top and middle

Link color visited change to match blue instead of grey

TODO:

  • Img-left - almost done
  • Expand - being worked on
  • Expand - examples in storybook worked on

Solving issue
Add which issue this pull-request solves by adding # plus the number of the issue (for example #123)
Fixes: AB#294

How to test
Add description how to test if possible

  • Start storybook
  • Compare to figma, both global component and the from the component page

Additional context
Add any other context about the pull-request here.
Some examples might come from some chat with niklas and not shown in the figma files

@mmexvr mmexvr added 🆕 feature New feature or request 🔧 component Issues related to components labels Mar 22, 2021
@mmexvr mmexvr self-assigned this Mar 22, 2021
helloimela
helloimela reviewed Mar 22, 2021
View reviewed changes
components/src/components/card/card.tsx Outdated

export class SddsCard {
// @Element() el: HTMLElement;
@Prop() clickable: boolean = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

clickable prop is not used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

helloimela
helloimela reviewed Mar 22, 2021
View reviewed changes
components/src/components/card/card.tsx Outdated
shadow: true
})

export class SddsCard {
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 use Card as the class name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

helloimela
helloimela reviewed Mar 22, 2021
View reviewed changes
components/src/components/card/card.stories.js Outdated
<div class="sdds-row demo">

<div class="sdds-col-xxlg-5 sdds-col-xlg-5 sdds-col-lg-5 sdds-hide-md">
<sdds-card clickable="true">
Copy link
Contributor

@helloimela helloimela Mar 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

From my understanding, at this moment sdds-card is not used for anything. Clickable styling is added from class "sdds-clickable". Is that correct? Maybe then remove sdds-card for now for this release to avoid confusion.
Because right now it feels like I need to add sdds-card clickable=true when it is doing nothing, and class sdds-clickable inside slot, which is not a suitable usecase of using slot (class should be added inside the web component, depends on the prop available). And of course, feels like a duplication for clickable prop :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the prop for now, it was related with expand solution, but no longer in use

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 also removed anything related with the web-component for now

helloimela
helloimela reviewed Mar 22, 2021
View reviewed changes
components/src/components/card/card.scss Outdated
@import '@scania/typography/dist/scss/mixins';
@import '@scania/typography/dist/scss/tokens';

.sdds-card {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use prefix for sdds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

helloimela
helloimela reviewed Mar 22, 2021
View reviewed changes
components/src/components/dropdown/readme.md Outdated
<sdds-dropdown-option value="option-3">Option 3</sdds-dropdown-option>
</sdds-dropdown>
```
# c-dropdown
Copy link
Contributor

Choose a reason for hiding this comment

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

merge from main and rebuild maybe? generated wrong readme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Was above the auto-generated doc part that had the be manually changed

helloimela
helloimela reviewed Mar 22, 2021
View reviewed changes
components/src/components/card/card.scss
}

//avatar
.#{$prefix}-card-header-avatar {
Copy link
Contributor

Choose a reason for hiding this comment

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

For the avatar, maybe we can add the correct wrapper? Right now it is a circle SVG, but in a real implementation in can be any image with border radius 50% right?. SO maybe add that to a div? Something like div border-radius 50% overflow hidden, So developer can add anything inside.

@niklasros
Copy link

Hand-cursor is missing on interactive cards.

@helloimela
Copy link
Contributor

helloimela commented Mar 22, 2021 •
edited
Loading

image
Picture from Figma for Button and icon used at the same time. Icon is on the right. I think this style is missing.

  • Can be added on next release
  • Please add a note / issue if added in next release

niklasros
niklasros approved these changes Mar 22, 2021
View reviewed changes
Copy link

@niklasros niklasros left a comment

Choose a reason for hiding this comment

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

Looks good!

helloimela
helloimela suggested changes Mar 22, 2021
View reviewed changes
components/src/components/card/card.scss
margin-right: 16px;
height: 36px;
width: 36px;
border-radius: 50%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to add overflow hidden.
And something like >> .sdds-card-avatar { img, svg { width:100% }}

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@mmexvr
Copy link
Contributor Author

mmexvr commented Mar 22, 2021

image
Picture from Figma for Button and icon used at the same time. Icon is on the right. I think this style is missing.

  • Can be added on next release
  • Please add a note / issue if added in next release

Adding issue on this

@mmexvr mmexvr mentioned this pull request Mar 22, 2021
Closed
helloimela
helloimela approved these changes Mar 22, 2021
View reviewed changes
Copy link
Contributor

@helloimela helloimela left a comment

Choose a reason for hiding this comment

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

Nice work!

@helloimela helloimela merged commit 6eeab71 into master Mar 22, 2021
@helloimela helloimela deleted the feature/card_component branch May 6, 2021 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@helloimela helloimela helloimela approved these changes

@niklasros niklasros niklasros approved these changes

Assignees

@mmexvr mmexvr

Labels
🔧 component Issues related to components 🆕 feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mmexvr @niklasros @helloimela