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

BB-764: Unified form ISBNs bug #1083

Merged
merged 3 commits into from
May 30, 2024
Merged

Conversation

insane-22
Copy link
Contributor

Problem

BB-764: On the unified form, ticking and then unticking the "Automatically add ISBN" checkbox results in the generated ISBN to be left over in the "edit identifiers" modal.
Unchecking should remove the auto-generated ISBN.

Solution

With the present code, what happens is that when the user checks the box for the first time, an action is dispatched to have the auto generated ISBN in the list of identifiers, but when he unchecks again, there was no direction to remove the added identifier. Hence, I have used this already defined removeIdentifierRow action:-
image

It requires rowId as a parameter, which we get using the addAutoIdentifier:-
image

After changes:-
https://github.com/metabrainz/bookbrainz-site/assets/116253210/ecfc1a2b-2f7e-45f0-817a-cc77243a27d9

Areas of Impact

src/client/unified-form/cover-tab/isbn-field.tsx
src/client/unified-form/interface/type.ts

Copy link
Member

@MonkeyDo MonkeyDo 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, thanks for fixing this!
Just a small naming suggestion to make the code a tad clearer.

You should also run npx eslint --fix src/client/unified-form/cover-tab/isbn-field.tsx to format the file, ESLint is complaining at the formatting otherwise

@@ -52,6 +53,7 @@ function mapStateToProps(rootState:State):ISBNStateProps {
}

function mapDispatchToProps(dispatch):ISBNDispatchProps {
let gett:dispatchResultProps|null;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a better name for this one would be:

Suggested change
let gett:dispatchResultProps|null;
let autoAddedISBN:dispatchResultProps|null;

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Perfect, thank you for another good bugfix!

@MonkeyDo MonkeyDo merged commit b2ab1c9 into metabrainz:master May 30, 2024
4 checks passed
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