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

add specification filter #11

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

IncognitaDev
Copy link

What does this PR do? *

this PR add prop to filter specifications inside group specifications

How to test it? *

create a product specifications table and pass filter prop to product-specifications like filter props os product-specification-group but with specificationNames instead specificationGroups.

Describe alternatives you've considered, if any. *

Related to / Depends on *

@IncognitaDev IncognitaDev requested a review from a team as a code owner October 25, 2021 13:53
@vtex-io-ci-cd
Copy link

vtex-io-ci-cd bot commented Oct 25, 2021

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch (backwards-compatible bug fixes)

  • Minor (backwards-compatible functionality)

  • Major (incompatible API changes)

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

@IncognitaDev IncognitaDev requested review from icazevedo, victorhmp and RodrigoTadeuF and removed request for a team October 25, 2021 13:53
@vtex-io-docs-bot
Copy link

Beep boop 🤖

I noticed you didn't make any changes at the docs/ folder

  • There's nothing new to document 🤔
  • I'll do it later 😞

In order to keep track, I'll create an issue if you decide now is not a good time

  • I just updated 🎉🎉

@igorbrasileiro
Copy link

Hey, can you leave a workspace to test it?

@IncognitaDev
Copy link
Author

react/ProductSpecification.tsx Outdated Show resolved Hide resolved
react/ProductSpecification.tsx Outdated Show resolved Hide resolved
Comment on lines 21 to 30
let specifications = group.specifications;

if (filter.specificationNames?.length > 0 && filter.type == 'show') {
specifications = specifications.filter((specification) =>
filter.specificationNames.includes(specification.originalName) )
}
if (filter.specificationNames?.length > 0 && filter.type == 'hide') {
specifications = specifications.filter((specification) =>
!filter.specificationNames.includes(specification.originalName) )
}
Copy link

@igorbrasileiro igorbrasileiro Oct 25, 2021

Choose a reason for hiding this comment

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

I think a way to remove this mutability. Feel free to argue 😅

What do you think about this suggestion?

const identityNoop = <T extends any>(x) => x // type the generics better, if possible.

type specificationFilterFunction = (specificationName: string) => boolean

interface UseFilterSpecification {
  specification: Array<{ originalName: string }>
  filters: {
    show: specificationFilterFunction,
    hide: specificationFilterFunction,
  }
}

const filterSpecification = ({ specification, filters: { show, hide } }: UseFilterSpecification) => {
   return specifications.filter(({ originalName }) => show(originalName) && hide(originalName))
}
Suggested change
let specifications = group.specifications;
if (filter.specificationNames?.length > 0 && filter.type == 'show') {
specifications = specifications.filter((specification) =>
filter.specificationNames.includes(specification.originalName) )
}
if (filter.specificationNames?.length > 0 && filter.type == 'hide') {
specifications = specifications.filter((specification) =>
!filter.specificationNames.includes(specification.originalName) )
}
const specifications = filterSpecification({
specifications: group.specifications,
filters: {
hide: filter.type === 'show' ? (specification) => filter.specificationNames.includes(specification) : identityNoop,
show: filter.type === 'hide' ? !(specification) => !filter.specificationNames.includes(specification) : identityNoop
}
})

That's it 😄 , you can rename or change or even disagree. If you feel more comfortable discussing in Portuguese, we can switch to PT-BR.

Choose a reason for hiding this comment

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

This suggestion seems not to escalate if we want to handle unlimited filters, but for now, it looks good.

Copy link
Author

@IncognitaDev IncognitaDev Oct 27, 2021

Choose a reason for hiding this comment

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

I think a way to remove this mutability. Feel free to argue 😅

What do you think about this suggestion?

const identityNoop = <T extends any>(x) => x // type the generics better, if possible.

type specificationFilterFunction = (specificationName: string) => boolean

interface UseFilterSpecification {
  specification: Array<{ originalName: string }>
  filters: {
    show: specificationFilterFunction,
    hide: specificationFilterFunction,
  }
}

const filterSpecification = ({ specification, filters: { show, hide } }: UseFilterSpecification) => {
   return specifications.filter(({ originalName }) => show(originalName) && hide(originalName))
}

That's it 😄 , you can rename or change or even disagree. If you feel more comfortable discussing in Portuguese, we can switch to PT-BR.

I make some adapts to fix errors but i'm not find the cause of this error: Argument of type 'string' is not assignable to parameter of type 'never'.

This just appear in hide function.

Comment on lines 21 to 30
let specifications = group.specifications;

if (filter.specificationNames?.length > 0 && filter.type == 'show') {
specifications = specifications.filter((specification) =>
filter.specificationNames.includes(specification.originalName) )
}
if (filter.specificationNames?.length > 0 && filter.type == 'hide') {
specifications = specifications.filter((specification) =>
!filter.specificationNames.includes(specification.originalName) )
}

Choose a reason for hiding this comment

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

This suggestion seems not to escalate if we want to handle unlimited filters, but for now, it looks good.

IncognitaDev and others added 3 commits October 25, 2021 19:40
Co-authored-by: Igor Brasileiro <brasileiro456@gmail.com>
Co-authored-by: Igor Brasileiro <brasileiro456@gmail.com>
@igorbrasileiro
Copy link

@IncognitaDev, we've created this [CONTRIBUTING.md] to help new contributions. Can you take a look there?

@IncognitaDev
Copy link
Author

@IncognitaDev, we've created this [CONTRIBUTING.md] to help new contributions. Can you take a look there?

Sure, but I don't find it. Wich repo contains ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants