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

New getter for category explanation text for the phase 2 store refactor #351

Merged
merged 7 commits into from
Feb 8, 2023

Conversation

jarmoza
Copy link
Contributor

@jarmoza jarmoza commented Feb 7, 2023

Closes #271.

This PR provides a new getter in the store getExplanation that attempts to retrieve the explanation key for a given category (via the categories store field). An accompanying unit test in store-getter-getExplanation.cy.js attempts to retrieve expected explanation text for a given category.

annot-explanation and its corresponding tests in annot-explanation.cy.js have been refactored for the new getter.

  • The former features a helper computed explanationText that either produces explanation text if the explanation key is present for a category or default text stored in the component. The placement of this text in the component is to keep in line with our precedent of maintaining UI-specific texts in the components and not the store.
  • Functionality for providing annot-explanation an explanation text via props has been removed (since its only prop in the refactor is now activeCategory) as has its test.

@jarmoza jarmoza added annotation page maint:refactor Simplifying or restructuring existing code or documentation. labels Feb 7, 2023
@jarmoza jarmoza requested a review from surchs February 7, 2023 22:16
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Thanks @jarmoza, looks good and passes! Left one comment inline, see if that was intentional. Another minor comment here: if the getExplanation getter would handle the "is explanation missing" by returning null if no explantion exists, then you could still provide a default value inside of the component but wouldn't have to worry about potentially missing keys - and you could also remove the mapState thing for categories.

return state.categories[p_category].explanation;
}
},
state: state
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to put both getters and state under the store object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@surchs Yes, I did. Why? Is there a reason not to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the getExplanation getter would handle the "is explanation missing" by returning null if no explantion exists, then you could still provide a default value inside of the component but wouldn't have to worry about potentially missing keys - and you could also remove the mapState thing for categories.

@surchs I'm having trouble understanding this comment. I think maybe there are two different ideas being confused here re: default value via props and default text for the UI. Can you break it down/reword? Or maybe pseudocode it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, I think adding them both under store is the way to go. sorry for not being clear: I asked because atm you are not organizing them both under the store object but then pass the state reference inside the store object. This looked odd to me so I wanted to know if you had meant to combine them both here to simplify the mock.

Copy link
Contributor Author

@jarmoza jarmoza Feb 8, 2023

Choose a reason for hiding this comment

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

I asked because atm you are not organizing them both under the store object but then pass the state reference inside the store object.

Ahh, yes. If you look inside the store initialization, state is referenced in getExplanation. I did it this way because referencing state before it is defined is a no-no. I wasn't sure because I thought it might have been my bias from compiled languages, but JavaScript acts the same way as a compiled one in this case. Just tried it out and it breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@surchs Scratch that. I think I understand your comment now.

If the getter in the store getExplanation returns null, I can check for that in explanationText in the component and provide the default text there - thus eliminating the need to use mapState for categories.

Good suggestion! I will apply it.

Also, afterwards I'm going to rename annotation-explanation.cy.js to annot-explanation.cy.js to keep the file name in line with other annotation page component tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

neat! and good catch on the name!

@jarmoza jarmoza merged commit 580520a into dev_components_talk_to_store_directly Feb 8, 2023
@jarmoza jarmoza deleted the jarmoza-271 branch February 8, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
annotation page maint:refactor Simplifying or restructuring existing code or documentation.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants