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

Remove lock in CategoryWidget if selected categories change #231

Merged
merged 4 commits into from
Dec 1, 2021

Conversation

padawannn
Copy link
Contributor

@padawannn padawannn commented Nov 30, 2021

  • For the category widget, unlock the widget if there are not a selected category

@coveralls
Copy link
Collaborator

coveralls commented Nov 30, 2021

Pull Request Test Coverage Report for Build 1526760560

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 69.069%

Totals Coverage Status
Change from base Build 1526754607: 0.08%
Covered Lines: 898
Relevant Lines: 1215

💛 - Coveralls

* @param {id} - sourceId of the filter to remove
* @param {owner} - widgetId of the filter to remove
*/
export const removeFilterByOwner = ({ id, owner }) => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Owner is the internal name, but I would probably suggest something more clear for the end user, like 'removeFilterFromWidget ({ sourceId, widgetId })

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I'm wondering if we need the sourceId, or if we can just iterate on all sources, without asking for the param, Does this make sense?

Can a widget affect several sources? I don't think so... right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I did it like this because it made my life easier for the integration with builder but it would be better to remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking better of it, this action is not necessary. Removed

@padawannn padawannn changed the title New action to remove filters by widget ID For the category widget, if the selected categories change the widget should not remain locked Dec 1, 2021
@VictorVelarde VictorVelarde changed the title For the category widget, if the selected categories change the widget should not remain locked Remove lock in CategoryWidget if selected categories change Dec 1, 2021
@VictorVelarde VictorVelarde force-pushed the padawannn/unlock-category-widget branch from e1c007c to 2fbbec5 Compare December 1, 2021 16:30
Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

LGTM

@VictorVelarde VictorVelarde merged commit 0eee58f into master Dec 1, 2021
@VictorVelarde VictorVelarde deleted the padawannn/unlock-category-widget branch December 1, 2021 16:32
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.

3 participants