BB-650: feat: Improve ISBN/Barcode detection for spaced, hyphenated input and add confirmation checkbox#1221
BB-650: feat: Improve ISBN/Barcode detection for spaced, hyphenated input and add confirmation checkbox#1221atharvsp02 wants to merge 1 commit intometabrainz:masterfrom
Conversation
…nfirmation checkbox for unusual values
atharvsp02
changed the title
MonkeyDo
left a comment
MonkeyDo
left a comment
There was a problem hiding this comment.
I think this is on the right track, but there are a few things I think could be changed. Generally, I think you should be trying to improve the existing mechanisms where possible rather than adding new methods.
| <FormCheck | ||
| checked={confirmed} | ||
| id={`identifier-confirm-${index}`} | ||
| label={`This doesn't look like a valid ${selectedIdentifierType}. Are you sure this is correct?`} |
There was a problem hiding this comment.
While this makes sense, I think the wording is strange for a checkbox which is usually an action.
I think either we show a bit of helptext above and change the checkbox label to "confirm" sort of text, or we rephrase the whole thing like "☑️ Confirm this invalid ISBN-13 (I know what I'm doing)"
Like this maybe?
| <Row> | ||
| <Col> | ||
| Preview Link: | ||
| <IdentifierLink typeId={typeValue} value={valueValue}/> |
There was a problem hiding this comment.
What do you think about formatting ISBNs like so in the preview?
There was a problem hiding this comment.
Yes, I think that is perfect
I have implemented the preview to display the ISBN with hyphens using the isbn3 library. I chose the library because manually implementing hyphenation looks difficult. Do you agree with this approach?
| </Col> | ||
| </Row> | ||
| )} | ||
| {valueValue && typeValue && !validateIdentifierValue(valueValue, typeValue, typeOptions) && ( |
There was a problem hiding this comment.
This should be limited to certain types only.
ISBN-10, ISBN-13 and Barcode
| value = result[1]; | ||
| let value = collapseWhiteSpaces(event.target.value); | ||
| const detectedType = detectIdentifierType(value); | ||
| let guessedType = null; |
There was a problem hiding this comment.
Would this work?
| let guessedType = null; | |
| const guessedType = types.find((type) => type.label === detectedType); |
Then checking if there is a match, otherwise the else block on line 182
| const result = new RegExp(guessedType.detectionRegex).exec(value); | ||
| value = result[1]; | ||
| let value = collapseWhiteSpaces(event.target.value); | ||
| const detectedType = detectIdentifierType(value); |
There was a problem hiding this comment.
Not a big fan of this new detectIdentifierType, why not improve the existing regexps that are used in guessIdentifierType instead?
Those are stored in the database, and could be improved based on the regexps here
https://github.com/metabrainz/bookbrainz-site/pull/1221/changes#diff-8841d7e2bffbb263a4a64fda61e9c9b35b28ee141e63e6b9e1b34063fd0a5c5fL58-L61
2 participants
Problem
Ticket: BB-650
ISBN and Barcode identifiers with spaces and hypens (e.g. 9 788205 174238) were not being detected or validated correctly. Users had to manually remove spaces before entering identifiers. Additionally, there was no way to submit unusual identifier values that don't match the expected regex format.
Solution
Areas of Impact
Entity Editor(Identifier Editor):
Unified Form (Cover Tab):
Screenshots
Before(Spaced and Unusual values)
After(Spaced and Unusual values)
Let me know if you have any suggestions or changes you’d like me to make.