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.

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

refactor: FilterMenu as a first step to remove ds-flex #3257

Closed
wants to merge 18 commits into from

Conversation

Tirokk
Copy link
Member

@Tirokk Tirokk commented Oct 5, 2020

alina-beck Authored by alina-beck
Mar 11, 2020
Merged Mar 27, 2020


🍰 Pullrequest

Migrates the ds-flex and ds-flex-item component from the styleguide by refactoring them out of the project and using CSS flexbox instead.

Issues

-fixes #3096

ToDo

  • add story (and test) for LabeledButton component

alina-beck and others added 18 commits March 25, 2020 11:02
Co-authored-by: mattwr18 <mattwr18@gmail.com>

- introduce LabeledButton component
- rename FilterMenu to HashtagsFilter and FilterPosts to FilterMenu
- remove ds-flex, ds-flex-item, ds-space, ds-heading
- not really happy with the design, and in mobile the divider border is
  hidden(!?)
Co-authored-by: Alina Beck <alina.beck@mail.com>
- Make the emotions filter similar in style to CategoriesFilter
@mattwr18 great work and great styling so far! ;)
fyi: I noticed there was a lot of duplicate CSS and the solution I came up with is this new component, using slots
@Tirokk
Copy link
Member Author

Tirokk commented Oct 7, 2020

cypress[bot] Authored by cypress[bot]
Mar 11, 2020




Test summary

66 0 0 0


Run details

Project Human-Connection
Status Passed
Commit 06985c2
Started Mar 26, 2020 7:08 PM
Ended Mar 26, 2020 7:32 PM
Duration 23:51 💡
OS Linux Ubuntu Linux - 16.04
Browser Firefox 68

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
Copy link
Member Author

Tirokk commented Oct 7, 2020

cypress[bot] Authored by cypress[bot]
Mar 11, 2020




Test summary

66 0 0 0


Run details

Project Human-Connection
Status Passed
Commit 77a53ef618 ℹ️
Started Mar 26, 2020 6:15 PM
Ended Mar 26, 2020 6:40 PM
Duration 24:59 💡
OS Linux Ubuntu Linux - 16.04
Browser Firefox 68

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

@click="filled = !filled"
/>
`,
}))
Copy link
Member Author

Choose a reason for hiding this comment

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

alina-beck Authored by alina-beck
Mar 24, 2020


Outdated (history rewrite) - original diff


@@ -0,0 +1,35 @@
+import { storiesOf } from '@storybook/vue'
+import helpers from '~/storybook/helpers'
+import LabeledButton from './LabeledButton.vue'
+
+storiesOf('Generic/LabeledButton', module)
+  .addDecorator(helpers.layout)
+  .add('default', () => ({
+    components: { LabeledButton },
+    data: () => ({
+      filled: false,
+    }),
+    template: `
+      <labeled-button
+        icon="check"
+        :filled="filled"
+        label="All"
+        @click="filled = !filled"
+      />
+    `,
+  }))
+
+  .add('filled', () => ({
+    components: { LabeledButton },
+    data: () => ({
+      filled: true,
+    }),
+    template: `
+      <labeled-button
+        icon="check"
+        :filled="filled"
+        label="All"
+        @click="filled = !filled"
+      />
+    `,
+  }))

@mattwr18 cool that you added the story! 💚

I think, since it's interactive, we don't really need the filled story and could instead maybe give the button in the default story the label Toggle me to make it clear that the button is reacting to input? What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

mattwr18 Authored by mattwr18
Mar 24, 2020


Great idea!

@Tirokk
Copy link
Member Author

Tirokk commented Oct 7, 2020

mattwr18 Authored by mattwr18
Mar 24, 2020


I'm happy with both the design and the code now. Loved working with you on this PR, @mattwr18! heart_eyes

yeah, it was really cool!! 😁
let's pair more!!

it('sets category button attribute `filled` when corresponding category is filtered', async () => {
getters['posts/filteredCategoryIds'] = jest.fn(() => ['cat9'])
wrapper = await Wrapper()
democracyAndPoliticsButton = wrapper.findAll('.categories-filter .item .base-button').at(2)
Copy link
Member Author

Choose a reason for hiding this comment

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

Mogge Authored by Mogge
Mar 27, 2020


Outdated (history rewrite) - original diff


@@ -0,0 +1,80 @@
+import { mount } from '@vue/test-utils'
+import Vuex from 'vuex'
+import CategoriesFilter from './CategoriesFilter'
+
+const localVue = global.localVue
+
+let wrapper, environmentAndNatureButton, democracyAndPoliticsButton
+
+describe('CategoriesFilter.vue', () => {
+  const mutations = {
+    'posts/TOGGLE_CATEGORY': jest.fn(),
+    'posts/RESET_CATEGORIES': jest.fn(),
+  }
+  const getters = {
+    'posts/filteredCategoryIds': jest.fn(() => []),
+  }
+
+  const mocks = {
+    $t: jest.fn((string) => string),
+  }
+
+  const Wrapper = () => {
+    const store = new Vuex.Store({ mutations, getters })
+    const wrapper = mount(CategoriesFilter, { mocks, localVue, store })
+    wrapper.setData({
+      categories: [
+        { id: 'cat4', name: 'Environment & Nature', icon: 'tree', slug: 'environment-nature' },
+        {
+          id: 'cat15',
+          name: 'Consumption & Sustainability',
+          icon: 'shopping-cart',
+          slug: 'consumption-sustainability',
+        },
+        {
+          id: 'cat9',
+          name: 'Democracy & Politics',
+          icon: 'university',
+          slug: 'democracy-politics',
+        },
+      ],
+    })
+    return wrapper
+  }
+
+  beforeEach(() => {
+    wrapper = Wrapper()
+  })
+
+  describe('mount', () => {
+    it('starts with all categories button active', () => {
+      const allCategoriesButton = wrapper.find('.categories-filter .sidebar .base-button')
+      expect(allCategoriesButton.attributes().class).toContain('--filled')
+    })
+
+    it('sets category button attribute `filled` when corresponding category is filtered', async () => {
+      getters['posts/filteredCategoryIds'] = jest.fn(() => ['cat9'])
+      wrapper = await Wrapper()
+      democracyAndPoliticsButton = wrapper.findAll('.categories-filter .item .base-button').at(2)

I do not like the .at(2)

.filter(c => c.text() === 'democracy-politics')

Copy link
Member Author

Choose a reason for hiding this comment

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

mattwr18 Authored by mattwr18
Mar 27, 2020


hey, thanks for the feedback! I agree with this, and have requested this change on other PRs as well, but I spent at least an hour, if not longer actually trying to get this to work, and it doesn't in this case because the button does not have a text, it has a separate <label>, and I couldn't workout how to filter the button based on it's sibling element.

What we could do is add a data-test="categories-button-${category.slug} and target the button that way. we remove data-test in production, so I think this would be better, what do you think?


describe('click on an "catetories-buttons" button', () => {
it('calls TOGGLE_CATEGORY when clicked', () => {
environmentAndNatureButton = wrapper.findAll('.categories-filter .item .base-button').at(0)
Copy link
Member Author

Choose a reason for hiding this comment

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

Mogge Authored by Mogge
Mar 27, 2020


Outdated (history rewrite) - original diff


@@ -0,0 +1,80 @@
+import { mount } from '@vue/test-utils'
+import Vuex from 'vuex'
+import CategoriesFilter from './CategoriesFilter'
+
+const localVue = global.localVue
+
+let wrapper, environmentAndNatureButton, democracyAndPoliticsButton
+
+describe('CategoriesFilter.vue', () => {
+  const mutations = {
+    'posts/TOGGLE_CATEGORY': jest.fn(),
+    'posts/RESET_CATEGORIES': jest.fn(),
+  }
+  const getters = {
+    'posts/filteredCategoryIds': jest.fn(() => []),
+  }
+
+  const mocks = {
+    $t: jest.fn((string) => string),
+  }
+
+  const Wrapper = () => {
+    const store = new Vuex.Store({ mutations, getters })
+    const wrapper = mount(CategoriesFilter, { mocks, localVue, store })
+    wrapper.setData({
+      categories: [
+        { id: 'cat4', name: 'Environment & Nature', icon: 'tree', slug: 'environment-nature' },
+        {
+          id: 'cat15',
+          name: 'Consumption & Sustainability',
+          icon: 'shopping-cart',
+          slug: 'consumption-sustainability',
+        },
+        {
+          id: 'cat9',
+          name: 'Democracy & Politics',
+          icon: 'university',
+          slug: 'democracy-politics',
+        },
+      ],
+    })
+    return wrapper
+  }
+
+  beforeEach(() => {
+    wrapper = Wrapper()
+  })
+
+  describe('mount', () => {
+    it('starts with all categories button active', () => {
+      const allCategoriesButton = wrapper.find('.categories-filter .sidebar .base-button')
+      expect(allCategoriesButton.attributes().class).toContain('--filled')
+    })
+
+    it('sets category button attribute `filled` when corresponding category is filtered', async () => {
+      getters['posts/filteredCategoryIds'] = jest.fn(() => ['cat9'])
+      wrapper = await Wrapper()
+      democracyAndPoliticsButton = wrapper.findAll('.categories-filter .item .base-button').at(2)
+      expect(democracyAndPoliticsButton.attributes().class).toContain('--filled')
+    })
+
+    describe('click on an "catetories-buttons" button', () => {
+      it('calls TOGGLE_CATEGORY when clicked', () => {
+        environmentAndNatureButton = wrapper.findAll('.categories-filter .item .base-button').at(0)

see above

describe('click on an "emotion-button" button', () => {
it('calls TOGGLE_EMOTION when clicked', () => {
const wrapper = Wrapper()
happyEmotionButton = wrapper.findAll('.emotion-button > .base-button').at(1)
Copy link
Member Author

Choose a reason for hiding this comment

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

Mogge Authored by Mogge
Mar 27, 2020


Outdated (history rewrite) - original diff


@@ -0,0 +1,64 @@
+import { mount } from '@vue/test-utils'
+import Vuex from 'vuex'
+import EmotionsFilter from './EmotionsFilter'
+
+const localVue = global.localVue
+
+let wrapper, happyEmotionButton
+
+describe('EmotionsFilter', () => {
+  const mutations = {
+    'posts/TOGGLE_EMOTION': jest.fn(),
+    'posts/RESET_EMOTIONS': jest.fn(),
+  }
+  const getters = {
+    'posts/filteredByEmotions': jest.fn(() => []),
+  }
+
+  const mocks = {
+    $t: jest.fn((string) => string),
+  }
+
+  const Wrapper = () => {
+    const store = new Vuex.Store({ mutations, getters })
+    return mount(EmotionsFilter, { mocks, localVue, store })
+  }
+
+  beforeEach(() => {
+    wrapper = Wrapper()
+  })
+
+  describe('mount', () => {
+    it('starts with all emotions button active', () => {
+      const allEmotionsButton = wrapper.find('.emotions-filter .sidebar .base-button')
+      expect(allEmotionsButton.attributes().class).toContain('--filled')
+    })
+
+    describe('click on an "emotion-button" button', () => {
+      it('calls TOGGLE_EMOTION when clicked', () => {
+        const wrapper = Wrapper()
+        happyEmotionButton = wrapper.findAll('.emotion-button > .base-button').at(1)

If possible, avoid .at()

it('sets the attribute `src` to colorized image', () => {
getters['posts/filteredByEmotions'] = jest.fn(() => ['happy'])
const wrapper = Wrapper()
happyEmotionButton = wrapper.findAll('.emotion-button > .base-button').at(1)
Copy link
Member Author

Choose a reason for hiding this comment

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

Mogge Authored by Mogge
Mar 27, 2020


Outdated (history rewrite) - original diff


@@ -0,0 +1,64 @@
+import { mount } from '@vue/test-utils'
+import Vuex from 'vuex'
+import EmotionsFilter from './EmotionsFilter'
+
+const localVue = global.localVue
+
+let wrapper, happyEmotionButton
+
+describe('EmotionsFilter', () => {
+  const mutations = {
+    'posts/TOGGLE_EMOTION': jest.fn(),
+    'posts/RESET_EMOTIONS': jest.fn(),
+  }
+  const getters = {
+    'posts/filteredByEmotions': jest.fn(() => []),
+  }
+
+  const mocks = {
+    $t: jest.fn((string) => string),
+  }
+
+  const Wrapper = () => {
+    const store = new Vuex.Store({ mutations, getters })
+    return mount(EmotionsFilter, { mocks, localVue, store })
+  }
+
+  beforeEach(() => {
+    wrapper = Wrapper()
+  })
+
+  describe('mount', () => {
+    it('starts with all emotions button active', () => {
+      const allEmotionsButton = wrapper.find('.emotions-filter .sidebar .base-button')
+      expect(allEmotionsButton.attributes().class).toContain('--filled')
+    })
+
+    describe('click on an "emotion-button" button', () => {
+      it('calls TOGGLE_EMOTION when clicked', () => {
+        const wrapper = Wrapper()
+        happyEmotionButton = wrapper.findAll('.emotion-button > .base-button').at(1)
+        happyEmotionButton.trigger('click')
+        expect(mutations['posts/TOGGLE_EMOTION']).toHaveBeenCalledWith({}, 'happy')
+      })
+
+      it('sets the attribute `src` to colorized image', () => {
+        getters['posts/filteredByEmotions'] = jest.fn(() => ['happy'])
+        const wrapper = Wrapper()
+        happyEmotionButton = wrapper.findAll('.emotion-button > .base-button').at(1)

s. o.

Copy link
Contributor

@Mogge Mogge 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 to me!! good job @alina-beck and @mattwr18

@Mogge Mogge closed this Oct 8, 2020
@ulfgebhardt ulfgebhardt deleted the pr3257head branch January 7, 2021 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants