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

Improve catalog container usability #968

Merged
merged 12 commits into from
Nov 25, 2022
Merged

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Nov 9, 2022

Description

Make the CatalogContainer usage a bit more generic. This will help us to reuse this component in other places where the catalog data is queried from wanda.
In order to achieve that, I have added a new reducer, selector and saga combo. This helps on testing side as well.

How was this tested?

Reducers, selectors and sagas tests added. In order to test the latest one, I have added the axios-mock-adapter to mock the api calls.

Here some references on how to test redux code (official docs):
https://redux.js.org/usage/writing-tests
https://redux-saga.js.org/docs/advanced/Testing/

@arbulu89 arbulu89 added the enhancement New feature or request label Nov 9, 2022
@arbulu89 arbulu89 marked this pull request as draft November 9, 2022 09:47
@arbulu89 arbulu89 force-pushed the improve-catalog-container-usability branch from 76881e5 to 5231fd9 Compare November 11, 2022 13:12
@arbulu89 arbulu89 marked this pull request as ready for review November 23, 2022 08:27
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

Just some changes then we can merge

assets/js/state/index.js Outdated Show resolved Hide resolved
assets/js/state/sagas/catalog.test.js Outdated Show resolved Hide resolved
assets/js/state/catalog_new.js Outdated Show resolved Hide resolved
const initialState = {
loading: true,
data: [],
error: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to have error: true|false and errorMessage, what do you think?

@arbulu89 arbulu89 force-pushed the improve-catalog-container-usability branch from f49efac to 600f3e9 Compare November 25, 2022 09:17
@arbulu89
Copy link
Contributor Author

Hey @dottorblaster ,
I have applied the requested changes, and even included some improvements to use the new wanda api code, so we have a centralized api usage.
I'm ommiting by now the error/errorMessage change if you don't mind. We already discussed offline the option to have some sort of Boolean selector, but we can include that afterwards

@arbulu89 arbulu89 force-pushed the improve-catalog-container-usability branch from 600f3e9 to d35fd4f Compare November 25, 2022 09:34
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

and even included some improvements to use the new wanda api code, so we have a centralized api usage

I forgot to tell you about that, you did the right thing!

@arbulu89 arbulu89 merged commit 580c312 into main Nov 25, 2022
@arbulu89 arbulu89 deleted the improve-catalog-container-usability branch November 25, 2022 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants