refactor(styleguide): migrate and redesign buttons #2562
Conversation
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
| @@ -74,7 +74,7 @@ describe('CommentForm.vue', () => { | |||
|
|
|||
| it('calls `clear` method when the cancel button is clicked', async () => { | |||
| wrapper.vm.updateEditorContent('ok') | |||
| await wrapper.find('.cancelBtn').trigger('submit') | |||
| await wrapper.find('[data-test="cancel-button"]').trigger('submit') | |||
There was a problem hiding this comment.
Authored by Tirokk
Dec 20, 2019
Outdated (history rewrite) - original diff
@@ -74,7 +74,7 @@ describe('CommentForm.vue', () => {
it('calls `clear` method when the cancel button is clicked', async () => {
wrapper.vm.updateEditorContent('ok')
- await wrapper.find('.cancelBtn').trigger('submit')
+ await wrapper.find('[data-test="cancel-button"]').trigger('submit')Nice 😍
| <base-button | ||
| :disabled="disabled && !update" | ||
| @click="handleCancel" | ||
| data-test="cancel-button" |
There was a problem hiding this comment.
Dec 20, 2019
Outdated (history rewrite) - original diff
@@ -1,28 +1,21 @@
<template>
- <ds-form v-model="form" @submit="handleSubmit">
+ <ds-form v-model="form" @submit="handleSubmit" class="comment-form">
<template slot-scope="{ errors }">
<ds-card>
<!-- with client-only the content is not shown -->
<hc-editor ref="editor" :users="users" :value="form.content" @input="updateEditorContent" />
- <ds-space />
- <ds-flex :gutter="{ base: 'small', md: 'small', sm: 'x-large', xs: 'x-large' }">
- <ds-flex-item :width="{ base: '0%', md: '50%', sm: '0%', xs: '0%' }" />
- <ds-flex-item :width="{ base: '40%', md: '20%', sm: '30%', xs: '30%' }">
- <ds-button
- :disabled="disabled && !update"
- ghost
- class="cancelBtn"
- @click.prevent="handleCancel"
- >
- {{ $t('actions.cancel') }}
- </ds-button>
- </ds-flex-item>
- <ds-flex-item :width="{ base: '40%', md: '20%', sm: '40%', xs: '40%' }">
- <ds-button type="submit" :loading="loading" :disabled="disabled || errors" primary>
- {{ $t('post.comment.submit') }}
- </ds-button>
- </ds-flex-item>
- </ds-flex>
+ <div class="buttons">
+ <base-button
+ :disabled="disabled && !update"
+ @click="handleCancel"
+ data-test="cancel-button"Same same 👆🏼
Wow @Tirokk, thank you! That was a very thorough review. 😍 |
I addressed all changes that @Tirokk suggested and added a few more improvements – with one exception: the |
| </ds-button> | ||
| </ds-flex-item> | ||
| </ds-flex> | ||
| <div class="buttons"> |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -1,28 +1,22 @@
<template>
- <ds-form v-model="form" @submit="handleSubmit">
+ <ds-form v-model="form" @submit="handleSubmit" class="comment-form">
<template slot-scope="{ errors }">
<ds-card>
<!-- with client-only the content is not shown -->
<hc-editor ref="editor" :users="users" :value="form.content" @input="updateEditorContent" />
- <ds-space />
- <ds-flex :gutter="{ base: 'small', md: 'small', sm: 'x-large', xs: 'x-large' }">
- <ds-flex-item :width="{ base: '0%', md: '50%', sm: '0%', xs: '0%' }" />
- <ds-flex-item :width="{ base: '40%', md: '20%', sm: '30%', xs: '30%' }">
- <ds-button
- :disabled="disabled && !update"
- ghost
- class="cancelBtn"
- @click.prevent="handleCancel"
- >
- {{ $t('actions.cancel') }}
- </ds-button>
- </ds-flex-item>
- <ds-flex-item :width="{ base: '40%', md: '20%', sm: '40%', xs: '40%' }">
- <ds-button type="submit" :loading="loading" :disabled="disabled || errors" primary>
- {{ $t('post.comment.submit') }}
- </ds-button>
- </ds-flex-item>
- </ds-flex>
+ <div class="buttons">I would typically suggest something a bit more descriptive, but I really like how you target this class as a grandchild of comment-form to apply CSS, so it doesn't make me nervous that the styling will be accidentally applied elsewhere.
Or that we be tempted to use scoped
There was a problem hiding this comment.
Jan 15, 2020
I know, it's counter-intuitive at first! Also my first time to use it like this but if we really follow the rscss suggestions and VueJS Styleguide all our components will have two-word-names and we'd only have these shorter and more generic class names as children of more specific classes. And I really like the readability of buttons in comment-form as opposed to BEM-style comment-form__buttons... 😉
| margin-top: 0; | ||
|
|
||
| > .counter-icon { | ||
| margin-right: $space-small; |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -50,3 +49,15 @@ export default {
},
}
</script>
+
+<style lang="scss">
+.comment-list {
+ > .title {
+ margin-top: 0;
+
+ > .counter-icon {
+ margin-right: 12px;I thought we wanted to avoid using magic numbers instead of design tokens, @alina-beck ??
There was a problem hiding this comment.
| filled | ||
| :disabled="!deleteEnabled" | ||
| @click="handleSubmit" | ||
| > |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -62,7 +62,13 @@
/>
</ds-flex-item>
<ds-flex-item :width="{ base: '100%', sm: '100%', md: '100%', lg: 1 }">
- <ds-button icon="trash" danger :disabled="!deleteEnabled" @click="handleSubmit">
+ <ds-button
+ icon="trash"
+ danger
+ filled
+ :disabled="!deleteEnabled"
+ @click="handleSubmit"
+ >why is this still ds-button?
actually, there are 10 files for me locally that still include ds-button... I will have a look at the other PR you put in... maybe there were bigger changes needed?
There was a problem hiding this comment.
Jan 15, 2020
yes, that's in the second PR. I know it's a bit confusing, I'll try to find a better solution next time than splitting things up into two PRs... sorry! 🤷♀
| <ds-button | ||
| <div class="follow-filter-button"> | ||
| <base-button | ||
| data-test="filter-by-followed" |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -13,17 +13,18 @@
<ds-flex-item width="10%" />
<ds-space margin-bottom="xx-small" />
<ds-flex-item width="100%">
- <div class="follow-button">
- <ds-button
+ <div class="follow-filter-button">
+ <base-button
+ name="filter-by-followed-authors-only"I know you didn't add this, but is this here only for testing purposes?
if so, wouldn't it be better to use data-test, which is removed in production?
There was a problem hiding this comment.
| import NotificationMenu from './NotificationMenu' | ||
|
|
||
| const localVue = global.localVue | ||
|
|
||
| localVue.filter('truncate', string => string) | ||
|
|
||
| config.stubs.dropdown = '<span class="dropdown"><slot /></span>' | ||
| config.stubs.dropdown = '<span class="dropdown"><slot :toggleMenu="() => null" /></span>' |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -1,11 +1,11 @@
-import { config, shallowMount } from '@vue/test-utils'
+import { config, mount } from '@vue/test-utils'
import NotificationMenu from './NotificationMenu'
const localVue = global.localVue
localVue.filter('truncate', string => string)
-config.stubs.dropdown = '<span class="dropdown"><slot /></span>'
+config.stubs.dropdown = '<span class="dropdown"><slot :toggleMenu="() => null" /></span>'cool trick!
| }) | ||
|
|
||
| it('renders primary button', () => { | ||
| it('renders the counter in red', () => { |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -130,12 +130,12 @@ describe('NotificationMenu.vue', () => {
it('displays the number of unread notifications', () => {
wrapper = Wrapper()
- expect(wrapper.find('ds-button-stub').text()).toEqual('2')
+ expect(wrapper.find('.count').text()).toEqual('2')
})
- it('renders primary button', () => {
+ it('renders the counter in red', () => {much better descriptions!
| </ds-button> | ||
| <base-button v-if="!notifications.length" class="notifications-menu" disabled ghost circle> | ||
| <counter-icon icon="bell" :count="unreadNotificationsCount" danger /> | ||
| </base-button> |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -1,12 +1,12 @@
<template>
- <ds-button v-if="!notifications.length" class="notifications-menu" disabled icon="bell">
- {{ unreadNotificationsCount }}
- </ds-button>
+ <base-button v-if="!notifications.length" class="notifications-menu" disabled ghost circle>
+ <counter-icon icon="bell" :count="unreadNotificationsCount" danger />
+ </base-button>cool! reusing counter-icon!!
| @@ -176,6 +176,8 @@ export default { | |||
| } | |||
|
|
|||
| .content-menu { | |||
| position: relative; | |||
| z-index: $z-index-post-card-link; | |||
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -176,6 +176,8 @@ export default {
}
.content-menu {
+ position: relative;
+ z-index: 99;I can't seem to figure out exactly why this is needed... if I comment out z-index, I don't seem to see any difference. with position: relative, the content menu has a hover affect that changes it to green, which I like by the way, but I don't notice any difference with or without the z-index.
Do we need it? If so, could we use a token?
There was a problem hiding this comment.
Jan 15, 2020
@mattwr18 you're starting to sound like me! 😜 Of course this should be a design token.
I added the z-index for the same reason we added it on the user-details: It needs to be on top of the post-link on the index page to be clickable.
| :loading="loading" | ||
| :disabled="disabled" | ||
| :ghost="!shouted" | ||
| :primary="shouted" | ||
| size="x-large" |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -1,12 +1,11 @@
<template>
<ds-space margin="xx-small" class="text-align-center">
- <ds-button
+ <base-button
:loading="loading"
:disabled="disabled"
- :ghost="!shouted"
- :primary="shouted"
- size="x-large"I like the new size of the shout button
| @@ -9,10 +9,18 @@ | |||
| @vdropzone-thumbnail="transformImage" | |||
| > | |||
| <div class="crop-overlay" ref="cropperOverlay" v-show="showCropper"> | |||
| <ds-button @click.stop.prevent="cropImage" class="crop-confirm" primary> | |||
| <base-button @click="cropImage" class="crop-confirm" filled> | |||
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -9,10 +9,18 @@
@vdropzone-thumbnail="transformImage"
>
<div class="crop-overlay" ref="cropperOverlay" v-show="showCropper">
- <ds-button @click.stop.prevent="cropImage" class="crop-confirm" primary>
+ <base-button @click="cropImage" class="crop-confirm" filled>I'm curious how you are getting away with removing the .stop.prevent from this...
we added it because we noticed that if you edit a post, and add a different image, and confirm crop, it would submit the form, but I've double checked and it doesn't happen anymore
There was a problem hiding this comment.
Jan 15, 2020
It's complicated and I don't fully understand it myself – it has to do with the implementation of ds-button vs base-button. The ds-button was wrapped in a div which handled the click event whereas the base-button has a native html button element at its root and can therefore use the native click event.
I tried to find more info on these handlers (stop, prevent, capture etc.) but the docs aren't very helpful, so I tried a few things and ended up with the above solution. If any of the buttons stop working after merging this PR it's one of the first things I would check, though. 😅
| <base-button size="small" icon="ellipsis-v" circle ghost /> | ||
| </div> | ||
| `, | ||
| })) |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -0,0 +1,85 @@
+import { storiesOf } from '@storybook/vue'
+import helpers from '~/storybook/helpers'
+import BaseButton from './BaseButton.vue'
+
+storiesOf('Generic/BaseButton', module)
+ .addDecorator(helpers.layout)
+
+ .add('default', () => ({
+ components: { BaseButton },
+ template: `
+ <div>
+ <base-button>Click me</base-button>
+ <base-button disabled>Disabled</base-button>
+ <base-button loading>Loading</base-button>
+ </div>
+ `,
+ }))
+
+ .add('icon', () => ({
+ components: { BaseButton },
+ template: `
+ <div>
+ <base-button icon="edit">With Text</base-button>
+ <base-button icon="bullhorn" />
+ <base-button icon="trash" disabled />
+ <base-button icon="trash" loading />
+ </div>
+ `,
+ }))
+
+ .add('circle', () => ({
+ components: { BaseButton },
+ template: `
+ <div>
+ <base-button circle icon="eye" />
+ <base-button circle>EN</base-button>
+ <base-button circle disabled icon="eye-slash" />
+ <base-button circle loading icon="eye-slash" />
+ </div>
+ `,
+ }))
+
+ .add('danger', () => ({
+ components: { BaseButton },
+ template: `
+ <div>
+ <base-button danger>Danger</base-button>
+ <base-button danger disabled>Disabled</base-button>
+ <base-button danger loading>Loading</base-button>
+ </div>
+ `,
+ }))
+
+ .add('filled', () => ({
+ components: { BaseButton },
+ template: `
+ <div>
+ <base-button filled>Filled</base-button>
+ <base-button filled danger>Filled Danger</base-button>
+ <base-button filled disabled>Disabled</base-button>
+ <base-button filled loading>Loading</base-button>
+ </div>
+ `,
+ }))
+
+ .add('small', () => ({
+ components: { BaseButton },
+ template: `
+ <div>
+ <base-button size="small">Small</base-button>
+ <base-button size="small" circle>S</base-button>
+ </div>
+ `,
+ }))
+
+ .add('ghost', () => ({
+ // TODO: add documentation --> ghost button should only be used for very special occasions
+ // e.g. for the ContentMenu + for the EditorMenuBarButtons
+ components: { BaseButton },
+ template: `
+ <div>
+ <base-button size="small" icon="ellipsis-v" circle ghost />
+ </div>
+ `,
+ }))really nice stories @alina-beck
| :class="buttonClass" | ||
| :disabled="loading" | ||
| :type="type" | ||
| @click.capture="event => $emit('click', event)" |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -0,0 +1,144 @@
+<template>
+ <button
+ :class="buttonClass"
+ :disabled="loading"
+ :type="type"
+ @click.capture="event => $emit('click', event)"maybe it has to do with this? 👆
There was a problem hiding this comment.
| buttonClass() { | ||
| let buttonClass = 'base-button' | ||
|
|
||
| if (this.$slots.default === undefined) buttonClass += ' --icon-only' |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -0,0 +1,144 @@
+<template>
+ <button
+ :class="buttonClass"
+ :disabled="loading"
+ :type="type"
+ @click.capture="event => $emit('click', event)"
+ >
+ <base-icon v-if="icon" :name="icon" />
+ <loading-spinner v-if="loading" />
+ <slot />
+ </button>
+</template>
+
+<script>
+import LoadingSpinner from '~/components/_new/generic/LoadingSpinner/LoadingSpinner'
+
+export default {
+ components: {
+ LoadingSpinner,
+ },
+ props: {
+ circle: {
+ type: Boolean,
+ default: false,
+ },
+ danger: {
+ type: Boolean,
+ default: false,
+ },
+ filled: {
+ type: Boolean,
+ default: false,
+ },
+ ghost: {
+ type: Boolean,
+ default: false,
+ },
+ icon: {
+ type: String,
+ },
+ loading: {
+ type: Boolean,
+ default: false,
+ },
+ size: {
+ type: String,
+ default: 'regular',
+ validator(value) {
+ return value.match(/(small|regular)/)
+ },
+ },
+ type: {
+ type: String,
+ default: 'button',
+ validator(value) {
+ return value.match(/(button|submit)/)
+ },
+ },
+ },
+ computed: {
+ buttonClass() {
+ let buttonClass = 'base-button'
+
+ if (this.$slots.default == null) buttonClass += ' --icon-only'why do you use == instead of === here?
There was a problem hiding this comment.
Jan 15, 2020
To also return true when it's undefined. But I just double-checked and I think slots are undefined when they're empty so it could be === undefined instead. 👍
| it('uses a primary button', () => { | ||
| expect(tag.classes()).toContain('ds-tag-primary') | ||
| it('renders the capped count with a plus', () => { | ||
| expect(count.text()).toEqual('99+') |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -5,33 +5,41 @@ import BaseIcon from '../BaseIcon/BaseIcon'
const localVue = global.localVue
describe('CounterIcon.vue', () => {
- let propsData, wrapper, tag
+ let propsData, wrapper, count
const Wrapper = () => {
return mount(CounterIcon, { propsData, localVue })
}
- describe('given a valid icon name and count', () => {
+ describe('given a valid icon name and count below 100', () => {
beforeEach(() => {
- propsData = { icon: 'comments', count: 1 }
+ propsData = { icon: 'comments', count: 42 }
wrapper = Wrapper()
- tag = wrapper.find('.ds-tag')
+ count = wrapper.find('.count')
})
- it('renders BaseIcon', () => {
+ it('renders the icon', () => {
expect(wrapper.find(BaseIcon).exists()).toBe(true)
})
it('renders the count', () => {
- expect(tag.text()).toEqual('1')
+ expect(count.text()).toEqual('42')
+ })
+ })
+
+ describe('given a valid icon name and count above 100', () => {
+ beforeEach(() => {
+ propsData = { icon: 'comments', count: 750 }
+ wrapper = Wrapper()
+ count = wrapper.find('.count')
})
- it('uses a round tag', () => {
- expect(tag.classes()).toContain('ds-tag-round')
+ it('renders the icon', () => {
+ expect(wrapper.find(BaseIcon).exists()).toBe(true)
})
- it('uses a primary button', () => {
- expect(tag.classes()).toContain('ds-tag-primary')
+ it('renders the capped count with a plus', () => {
+ expect(count.text()).toEqual('99+')really like this!
| Report Details | ||
| </ds-button> | ||
| </counter-icon> | ||
| <counter-icon icon="bell" :count="0" /> |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -2,18 +2,33 @@ import { storiesOf } from '@storybook/vue'
import helpers from '~/storybook/helpers'
import CounterIcon from './CounterIcon.vue'
-storiesOf('CounterIcon', module)
+storiesOf('Generic/CounterIcon', module)
.addDecorator(helpers.layout)
- .add('flag icon with button in slot position', () => ({
+
+ .add('default', () => ({
+ components: { CounterIcon },
+ template: `
+ <counter-icon icon="flag" :count="3" />
+ `,
+ }))
+
+ .add('high count', () => ({
+ components: { CounterIcon },
+ template: `
+ <counter-icon icon="comments" :count="150" />
+ `,
+ }))
+
+ .add('danger', () => ({
+ components: { CounterIcon },
+ template: `
+ <counter-icon icon="bell" :count="42" danger />
+ `,
+ }))
+
+ .add('count is 0', () => ({
components: { CounterIcon },
- data() {
- return { icon: 'flag', count: 3 }
- },
template: `
- <counter-icon icon="pizza" :count="count">
- <ds-button ghost primary>
- Report Details
- </ds-button>
- </counter-icon>
+ <counter-icon icon="bell" :count="0" />cool stuff here!
I like that I can see what they look like without needing to create 100 notifications, for example
I did notice that the counter is neutral when inactive... which makes sense!
| .add('default', () => ({ | ||
| components: { LoadingSpinner }, | ||
| template: '<loading-spinner />', | ||
| })) |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -0,0 +1,11 @@
+import { storiesOf } from '@storybook/vue'
+import helpers from '~/storybook/helpers'
+import LoadingSpinner from './LoadingSpinner.vue'
+
+storiesOf('Generic/LoadingSpinner', module)
+ .addDecorator(helpers.layout)
+
+ .add('default', () => ({
+ components: { LoadingSpinner },
+ template: '<loading-spinner />',
+ }))it just spins and spins!! 😛
| .add('default', () => ({ | ||
| components: { LoadingSpinner }, | ||
| template: '<loading-spinner />', | ||
| })) |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -0,0 +1,11 @@
+import { storiesOf } from '@storybook/vue'
+import helpers from '~/storybook/helpers'
+import LoadingSpinner from './LoadingSpinner.vue'
+
+storiesOf('Generic/LoadingSpinner', module)
+ .addDecorator(helpers.layout)
+
+ .add('default', () => ({
+ components: { LoadingSpinner },
+ template: '<loading-spinner />',
+ }))it just spins and spins!! 😛
| transform: rotate(2160deg); | ||
| } | ||
| } | ||
| </style> |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -0,0 +1,48 @@
+<template>
+ <svg viewBox="0 0 50 50" class="loading-spinner">
+ <circle cx="25" cy="25" r="20" class="circle" />
+ </svg>
+</template>
+
+<script>
+export default {
+ name: 'LoadingSpinner',
+}
+</script>
+
+<style lang="scss">
+.loading-spinner {
+ height: $size-button-base;
+ overflow: hidden;
+ stroke: currentColor;
+ animation: rotate 16s linear infinite;
+
+ > .circle {
+ fill: none;
+ stroke-width: 5;
+ stroke-linecap: round;
+ animation: dash 1.5s ease-in-out infinite;
+ }
+}
+
+@keyframes dash {
+ 0% {
+ stroke-dasharray: 1, 150;
+ stroke-dashoffset: 0;
+ }
+ 50% {
+ stroke-dasharray: 90, 150;
+ stroke-dashoffset: -35;
+ }
+ 100% {
+ stroke-dasharray: 90, 150;
+ stroke-dashoffset: -124;
+ }
+}
+
+@keyframes rotate {
+ 100% {
+ transform: rotate(2160deg);
+ }
+}
+</style>cool new reusable component!
| <ds-button @click="back" :disabled="!hasPrevious" icon="arrow-left" primary /> | ||
| </ds-flex-item> | ||
| </ds-flex> | ||
| <hc-paginate :hasNext="hasNext" :hasPrevious="hasPrevious" @next="next" @back="back" /> |
There was a problem hiding this comment.
Jan 14, 2020
Outdated (history rewrite) - original diff
@@ -50,14 +50,7 @@
{{ scope.row.createdAt | dateTime }}
</template>
</ds-table>
- <ds-flex direction="row-reverse">
- <ds-flex-item width="50px">
- <ds-button @click="next" :disabled="!hasNext" icon="arrow-right" primary />
- </ds-flex-item>
- <ds-flex-item width="50px">
- <ds-button @click="back" :disabled="!hasPrevious" icon="arrow-left" primary />
- </ds-flex-item>
- </ds-flex>
+ <hc-paginate :hasNext="hasNext" :hasPrevious="hasPrevious" @next="next" @back="back" />ahhh, thanks... I must have forgotten to refactor this
There was a problem hiding this comment.
I feel like I asked for some changes, but I'm going to approve, cause maybe you can give reasons why, or you have changed them in the other PR
Great job!! Really love it! would love to get the client, @DennisHack, to sign off on the design changes, if he hasn't already
🍰 Pullrequest
CounterIcon(because it was wrapped around a button but I would argue it should be the other way round 😉)Old buttons
New buttons
Please start
storybookto see them in action!Motivation
secondarybutton because it is never used on the site and would disrupt the design in its current state. I don't think we need the blue color at all.disabledwhich can be very confusing. By the way, the vast majority of buttons on the network is set asprimary(filled in green) anyway.ghostbuttons as well. They are very rarely used and I think the new design of thedefaultbutton is a good replacement – especially sinceghostbuttons are only ever used in combination withprimaryordangerbuttons.primary-lightandprimary-dark, same fordanger), so thehoverandactivestates are now more easily distinguishable.circleshapes which was impossible before.Issues
Questions
Testing: We should make this decision as a team, but I'm especially interested what @mattwr18 has to say – because you added tests for
CounterIcon: What should be covered in component tests, especially forgenericcomponents?I think it would make sense to test logic, but only if there is any (in case of the
CounterIconI added a cap for the counter, so we could make sure that count values below100are displayed at their actual value and counts above as99+) but I don't think it is necessary to test that all the elements are rendered. If that is something we do want to test I would maybe suggest thestorybook snapshotsinstead. What do you think?Icon Button Design: Do you think there is a use case for the below button style or should
icon-onlybuttons by default becircles?ToDo
<ds-button>with<base-button>throughout the projectscss variablesinBaseComponents