-
Notifications
You must be signed in to change notification settings - Fork 86
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
Feat/badge #394
Feat/badge #394
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, Denis - I've left comments, please take a look and let me know what do you think.
Can you add visual tests as well, please?
background: var(--color-badge); | ||
} | ||
|
||
.badge.badge--color-magenta { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do not use magenta as a name - use primary instead
} | ||
|
||
.badge.badge--color-magenta { | ||
background: #e20074; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use tokens
}) | ||
export class Badge { | ||
/** (optional) Variant size of the badge itself */ | ||
@Prop({ mutable: true }) size: 'big' | 'small' = 'big'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use large
instead of big
- similar to the button: https://github.com/telekom/scale/blob/main/packages/components/src/components/button/button.tsx#L25
</div> | ||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need all these linebreaks?
``` | ||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove the extra linebreaks, please?
|
||
export const Template = (args, { argTypes }) => ({ | ||
components: { ScaleBadge }, | ||
props: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's just make this props: ScaleBadge.props
- the rest is not needed. Same goes for the other templates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please take a look here: no need to spread ScaleBadge.props
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same goes for all the other occurrences in this file. thanks!
</Canvas> | ||
|
||
```html | ||
<scale-badge style="--badge-color:blue" size:"small" > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the style attribute does nothing, right? let's remove it to avoid confusing readers
</Canvas> | ||
|
||
```html | ||
<scale-badge size="big" style="--badge-color:blue" rotation="0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you fix the indentation, please?
</Canvas> | ||
|
||
```html | ||
<scale-badge size="big" color="magenta" rotation="0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Denis, thanks for your work on this. I've left comments where your attention is needed. Thanks again.
components: [Badge], | ||
html: `<scale-badge | ||
size ="big" | ||
color ="magenta" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be primary
}); | ||
|
||
expect(page.rootInstance.size).toBe('big'); | ||
expect(page.rootInstance.color).toBe('magenta'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be primary
|
||
| Property | Attribute | Description | Type | Default | | ||
| ---------- | ---------- | ----------------------------------------------- | ------------------------------------------- | ----------- | | ||
| `color` | `color` | (optional) Variant color/filling of the badge | `"black" \| "blue" \| "magenta" \| "white"` | `undefined` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
magenta should be primary - did you forget to build the components?
| ---------- | ---------- | ----------------------------------------------- | ------------------------------------------- | ----------- | | ||
| `color` | `color` | (optional) Variant color/filling of the badge | `"black" \| "blue" \| "magenta" \| "white"` | `undefined` | | ||
| `rotation` | `rotation` | (optional) Variant rotation of the badge/circle | `number` | `0` | | ||
| `size` | `size` | (optional) Variant size of the badge itself | `"big" \| "small"` | `'big'` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
big should be large - forgot to build?
<div class="agent-states--grid" data-category="Text only"> | ||
<div class="agent-states--item"> | ||
<p class="agent-states--label">Primary - Large</p> | ||
<scale-badge size="big" color="primary" rotation="0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
big should be large instead
|
||
export const Template = (args, { argTypes }) => ({ | ||
components: { ScaleBadge }, | ||
props: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please take a look here: no need to spread ScaleBadge.props
|
||
export const Template = (args, { argTypes }) => ({ | ||
components: { ScaleBadge }, | ||
props: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same goes for all the other occurrences in this file. thanks!
<scale-badge style="--badge-color:blue" size:"small" > | ||
<div> | ||
<div | ||
style="text-align:center; font-size:20px; font-family:TeleNeo; color:blwhiteack; font-weight:400;" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix the typo
<scale-badge color:"blue" size:"small" rotation:"33" > | ||
<div> | ||
<div | ||
style="text-align:center; font-size:20px; font-family:TeleNeo; color:blwhiteack; font-weight:400;" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix the typo
@@ -0,0 +1,13 @@ | |||
<div style="display: inline-flex; align-items: center; justify-content: space-between; width: 100%;"> | |||
<h1>Badge</h1> | |||
<img src="assets/aa.png" alt="Accessible AA" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use the beta badge in the German file too. thanks!
Hi Daniel, I think everything seems to be good know. I have my fingers crossed. Thank you again so much for the help, time and patience! |
Hi Daniel, as mentioned in SLACK: |
Fixed the Font-Family/Size |
* chore: implement password protected staging server, tidy up scripts (#434) * feat: badge (#394) * feat: init badge * refactor: everything * feat: init hbs * refactor: inline style to css class mapping * refactor: deleting global variables * refactor: removing border * refactor: deleting old test cases * added storybook * Storybook & Handlebar done * copied and added the index.html from main branch * prettier & lint * fixed errors according to Daniel check, after merge request 1 * format and lint * changes commited according to Daniels check from first MR * applied change according to Daniel, from merge request 2. * design tokens added * Font-Family fixed * added design token for font-family * Edited storybook - Font Co-authored-by: marvinLaubenstein <[email protected]> * ci(bernerslee): remove s3 pipeline * Sketch/readonly input state (#373) * feat(sketch): add readonly states to text-fields * feat(sketch): add readonly states for textarea * feat(textarea): add readonly styles * refactor(sketch): add readonly to smybol name state resolver * chore(sketch): update sketch symbol DB * feat: implement scaleBeforeClose event in Modal (#430) * Update includes and symbol names (#372) * chore: update static sketch symbols * chore: update color swatches and missing readonly state * refactor: change symbol hirarchy for textarea / text-field * refactor: update readonly assignments * fix(button): add icon scaling and resiziable layout of slot elements * fix(dropdown/sketch): improve rendering for select element * chore: autoformat files * fix: datepicker not updating on outside value change (#441) * refactor(rating-stars): rework with input range approach also make the whole thing more modular with parts * feat(rating-stars): add half star rating * feat(rating-stars): use correct aria value and add better half visuals * refactor: add size, editable label; css * feat: only first star can clear all stars * refactor: prop renaming; remove watcher; * test: writing tests * refactor: renaming value to rating * refactor: stories * feat: implement onTouchEnd * feat(rating-stars): add readonly state and a11y features * feat(rating-stars): simplify revert logic / handle minRating input * feat: add prop readonly * feat: edit PR requests * feat: firstStarSelected with a default value of false * feat: input gets focus onClick * feat(rating-stars): add sketch generation and adjust input handling * fix: revert handmade sketch includes * test: update snapshots Co-authored-by: Kutlovcidenis <[email protected]> Co-authored-by: marvinLaubenstein <[email protected]> Co-authored-by: Daniel Beck <[email protected]> Co-authored-by: Stefan Kopco <[email protected]> Co-authored-by: Calvin Schröder <[email protected]> Co-authored-by: Christian Pajung <[email protected]>
Hi Daniel, as disscused my merge request.
From my side, everything is good. But if not, just let me know.
Always eager to learn new/better ways :)
BR
Denis