refactor(styleguide): migrate and redesign buttons #2562

alina-beck · 2019-12-18T07:25:16Z

🍰 Pullrequest

Migration of styleguide buttons + improved code and functionality + design changes
Migration of styleguide spinner (because it is used in the buttons)
Refactoring of the 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 storybook to see them in action!

Motivation

I got rid of the secondary button 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.
I changed the design of the default button because with grey text on grey background it looks disabled which can be very confusing. By the way, the vast majority of buttons on the network is set as primary (filled in green) anyway.
This is part of the reason why I decided to remove the ghost buttons as well. They are very rarely used and I think the new design of the default button is a good replacement – especially since ghost buttons are only ever used in combination with primary or danger buttons.
I introduced new color shades (primary-light and primary-dark, same for danger), so the hover and active states are now more easily distinguishable.
Buttons can now explicitly be set as circle shapes which was impossible before.

Issues

relates 🌟 [EPIC] Styleguide Migration #2074

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 for generic components?
I think it would make sense to test logic, but only if there is any (in case of the CounterIcon I added a cap for the counter, so we could make sure that count values below 100 are displayed at their actual value and counts above as 99+) 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 the storybook snapshots instead. What do you think?
Icon Button Design: Do you think there is a use case for the below button style or should icon-only buttons by default be circles?

ToDo

get the design approved 😁
replace <ds-button> with <base-button> throughout the project
use scss variables in BaseComponents

cypress · 2019-12-18T08:59:14Z

Test summary

51 • 0 • 0 • 0

Run details


Project	Human-Connection
Status	Passed
Commit	`42e2e39`
Started	Jan 15, 2020 11:00 AM
Ended	Jan 15, 2020 11:12 AM
Duration	11:42 💡
OS	Linux Ubuntu Linux - 16.04
Browser	Chromium 79

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

Tirokk

Wow @alina-beck !
That looks really nice and is great in general !!! 😍💫👍🏼

Examples …

The buttons in the editor looking great!
… for the categories as well!

I just started the review and will add up what I’ve seen so far:

On the frontend I found:

On the new terms and conditions page hover the button looks not nice. The text is unreadable:

On the create post page Cancel looks on hover the same as Save not hovered:

If I hover an emoticon on the post page nothing changes at all before I click:

The content menu is not working on a post card:

Create post button is not working on the users profile page:

Shall the notifications red number circle be cut?

Emoticons are a little bit small in the filter menu and not aligned with the Users I follow button:

… review in progress …

Tirokk · 2019-12-20T08:58:19Z

webapp/components/CommentForm/CommentForm.spec.js

@@ -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')


Tirokk · 2019-12-20T08:59:08Z

webapp/components/CommentForm/CommentForm.vue

+          <base-button
+            :disabled="disabled && !update"
+            @click="handleCancel"
+            data-test="cancel-button"


Same same 👆🏼

alina-beck · 2020-01-06T04:59:01Z

Wow @Tirokk, thank you! That was a very thorough review. 😍
I'll go fix it now. 😉

alina-beck · 2020-01-14T14:01:18Z

I addressed all changes that @Tirokk suggested and added a few more improvements – with one exception: the emotions buttons, both on the post page and the filter menu, are fixed in a follow-up PR (see #2582) because the markup was quite messed up and needed some bigger changes. 😉

all changes addressed

mattwr18

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

mattwr18 · 2020-01-14T17:12:59Z

webapp/components/CommentForm/CommentForm.vue

-            </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

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... 😉

mattwr18 · 2020-01-14T17:13:54Z

webapp/components/CommentList/CommentList.vue

+    margin-top: 0;
+
+    > .counter-icon {
+      margin-right: 12px;


I thought we wanted to avoid using magic numbers instead of design tokens, @alina-beck ??

got me... 😊 we do!

mattwr18 · 2020-01-14T17:19:20Z

webapp/components/DeleteData/DeleteData.vue

+                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?

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! 🤷‍♀

mattwr18 · 2020-01-14T17:22:50Z

webapp/components/FilterPosts/GeneralFilterMenuItems.vue

-              <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?

ha, I'll go check!

mattwr18 · 2020-01-14T17:29:45Z

webapp/components/NotificationMenu/NotificationMenu.spec.js

 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!

mattwr18 · 2020-01-14T17:57:01Z

webapp/components/_new/generic/CounterIcon/CounterIcon.spec.js

-    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!

mattwr18 · 2020-01-14T18:13:07Z

webapp/components/_new/generic/CounterIcon/CounterIcon.story.js

-          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!

mattwr18 · 2020-01-14T18:13:49Z

webapp/components/_new/generic/LoadingSpinner/LoadingSpinner.story.js

+  .add('default', () => ({
+    components: { LoadingSpinner },
+    template: '<loading-spinner />',
+  }))


it just spins and spins!! 😛

mattwr18 · 2020-01-14T18:14:13Z

webapp/components/_new/generic/LoadingSpinner/LoadingSpinner.vue

+    transform: rotate(2160deg);
+  }
+}
+</style>


cool new reusable component!

mattwr18 · 2020-01-14T18:15:06Z

webapp/pages/admin/users.vue

-          <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

alina-beck added 10 commits December 10, 2019 13:17

set up css resets

8f70ce5

add BaseButton component

2c2f69f

add button states

dd89c26

sort storybook stories alphabetically

92856af

rearrange button stories

557fa32

style button variants

ab9ad46

migrate loading spinner

69a69ad

style button loading state

93233a5

rearrange BaseButton stories

5a30001

refactor counter icon

7568837

alina-beck self-assigned this Dec 18, 2019

alina-beck requested review from DennisHack and a team December 18, 2019 07:25

Merge branch 'master' into migrate-styleguide-buttons

c83f158

alina-beck added 11 commits December 18, 2019 22:55

add button size small

63549d9

add button type to allow submit buttons

d656043

add ghost style for special use cases

7cf8aec

replace ds-button with base-button (wip)

7c7a800

continue to replace ds-button with base-button (wip)

85f6288

replace even more ds-buttons with base-buttons (wip)

ee11f4e

add color variants to CounterIcon

72f2aff

replace another batch of ds-buttons with base-buttons (wip)

ea612bd

fix failing component tests

ad27404

let eslint do some magic

56b1a74

reduce CounterIcon tests to only test logic

5c1242b

alina-beck mentioned this pull request Dec 19, 2019

refactor(styleguide): improve emotion buttons and header responsiveness #2582

Merged

3 tasks

alina-beck changed the title ~~[WIP] Migrate Styleguide Buttons~~ Migrate Styleguide Buttons Dec 19, 2019

Tirokk changed the title ~~Migrate Styleguide Buttons~~ 🍰 Migrate Styleguide Buttons Dec 19, 2019

Tirokk previously requested changes Dec 19, 2019

View reviewed changes

Tirokk reviewed Dec 20, 2019

View reviewed changes

alina-beck mentioned this pull request Jan 7, 2020

🎨 [Styleguide Migration] Buttons #2684

Closed

alina-beck added 11 commits January 8, 2020 13:49

make content-menu clickable on post-card

cc38fa2

wrap create-post link around button on user profile

5d2af86

allow overflow for notifications counter

53a5040

fix styling of terms-and-conditions button

be30b55

manage button states and color schemes with mixin

1b9249c

rename primary to filled

34006a2

make cancel buttons red

93b6323

fix tests and lint errors

04e5ecc

use scss variables in base button component

b71638d

use scss tokens for counter icon and loading spinner

70d632e

fix cypress tests (wip)

1b127f1

alina-beck requested review from Tirokk and a team January 14, 2020 13:54

alina-beck changed the title ~~🍰 Migrate Styleguide Buttons~~ refactor(styleguide): migrate and redesign buttons Jan 14, 2020

mattwr18 approved these changes Jan 14, 2020

View reviewed changes

alina-beck added 2 commits January 15, 2020 11:55

follow @mattwr18 code review

150d040

fix lint errors in test

7c89404

alina-beck merged commit 56a3822 into master Jan 15, 2020

Tirokk mentioned this pull request Oct 7, 2020

🎨 [Styleguide Migration] Buttons Ocelot-Social-Community/Ocelot-Social#2684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(styleguide): migrate and redesign buttons #2562

refactor(styleguide): migrate and redesign buttons #2562

alina-beck commented Dec 18, 2019 •

edited

Loading

cypress bot commented Dec 18, 2019 •

edited

Loading

Tirokk left a comment •

edited

Loading

Tirokk Dec 20, 2019

Tirokk Dec 20, 2019

alina-beck commented Jan 6, 2020

alina-beck commented Jan 14, 2020

mattwr18 left a comment

mattwr18 Jan 14, 2020

alina-beck Jan 15, 2020

mattwr18 Jan 14, 2020

alina-beck Jan 15, 2020

mattwr18 Jan 14, 2020

alina-beck Jan 15, 2020

mattwr18 Jan 14, 2020

alina-beck Jan 15, 2020

mattwr18 Jan 14, 2020

mattwr18 Jan 14, 2020

mattwr18 Jan 14, 2020

mattwr18 Jan 14, 2020

mattwr18 Jan 14, 2020

mattwr18 Jan 14, 2020

refactor(styleguide): migrate and redesign buttons #2562

refactor(styleguide): migrate and redesign buttons #2562

Conversation

alina-beck commented Dec 18, 2019 • edited Loading

🍰 Pullrequest

Issues

Questions

ToDo

cypress bot commented Dec 18, 2019 • edited Loading

Test summary

Run details

Tirokk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alina-beck commented Jan 6, 2020

alina-beck commented Jan 14, 2020

mattwr18 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alina-beck commented Dec 18, 2019 •

edited

Loading

cypress bot commented Dec 18, 2019 •

edited

Loading

Tirokk left a comment •

edited

Loading