Clean up and rename computed variables #10307
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10307 +/- ##
==========================================
+ Coverage 17.44% 17.59% +0.14%
==========================================
Files 89 88 -1
Lines 4792 4769 -23
Branches 848 847 -1
==========================================
+ Hits 836 839 +3
+ Misses 3436 3411 -25
+ Partials 520 519 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Awesome, thanks for tackling @schu96 !! I think we can do a bit more to making this component less aware of the isEdition logic; by making it less tightly coupled to this notion, we should be able to DRY up a few of these methods, since there are a number of shared patterns. A few spots of feedback for you! Let me know if you have any questions 😊
cdrini
added
the
Needs: Submitter Input
github-actions
bot
removed
the
Needs: Submitter Input
schu96
changed the title
| if (this.secondary_identifiers) { | ||
| const secondConfigs = JSON.parse(decodeURIComponent(this.secondary_identifiers)); | ||
| return Object.fromEntries(secondConfigs.map(e => [e.name, e])); | ||
| popularIdDict: function() { |
There was a problem hiding this comment.
Ok, looking great! This can now just be a list, since we never have to do a look-up by identifier name, which makes this code much tighter!
popularIds: function() {
const popularIdNames = JSON.parse(decodeURIComponent(this.popular_ids));
return this.parsedConfig
.filter((config) => popularConfig.includes(config.name));
},| if (this.secondary_identifiers) { | ||
| const secondConfigs = JSON.parse(decodeURIComponent(this.secondary_identifiers)); | ||
| return Object.fromEntries(secondConfigs.map(e => [e.name, e])); | ||
| popularIdDict: function() { |
There was a problem hiding this comment.
And to keep the naming consistent, let's renaming the property to popular_ids_string ; that way any time we have a JSON/string input, it ends with foo_string, and then the parsed non-string version is just foo.
| return Object.fromEntries(popularConfigs.map(e => [e.name, e])); | ||
| } | ||
| return {}; | ||
| parsedConfig: function() { |
There was a problem hiding this comment.
So this can then be renamed to idConfig, and we don't specify parsed
| return Object.fromEntries(betterConfigs.map(e => [e.name, e])); | ||
| } | ||
| return Object.fromEntries(parsedConfigs['identifiers'].map(e => [e.name, e])); | ||
| identifierConfigsByKey: function() { |
There was a problem hiding this comment.
This now also looks un-needed and can be removed ; we can replace where this is used with the parsedConfig computed.
cdrini
added
the
Needs: Submitter Input
| @@ -447,7 +447,7 @@ | |||
| $ set1 = [id_dict[name] for name in popular if name in id_dict and name not in exclude] | |||
| $ set2 = sorted([id for id in edition_config.identifiers if id.name not in popular and id.name not in exclude], key=lambda id:id.label) | |||
There was a problem hiding this comment.
Oh and set1 and set2 might now no longer be used!
Closes #10301
Refactor
Technical
Remove computed
isEditionvariable and usesaveIdentifiersAsListto ensure component still functions properly when saving Author identifiers vs Edition/Work identifiers.Removes props initially used to create an ordered dropdown list for identifiers, add a popular identifier list within the IdentifiersInput.vue for easier future maintenance.
Testing
Run
docker compose up -dand thendocker-compose run --rm home npx vue-cli-service build --watch --no-clean --mode production --dest static/build/components/production --target wc --name ol-IdentifiersInput openlibrary/components/IdentifiersInput.vueSign in using the OpenLibrary dev account or as a patron
Navigate to an edition and click on the edit button. Click on the identifiers component and check that the identifiers dropdown list contains two sets of identifiers separated by a non-selectable option with three dashes.
Within the same edit page, select the Work Details tab. Click on the identifiers component and verify that the work identifiers dropdown list contains only one set of identifiers without the three dashes option.
Navigate to an author page and click on the edit button. The dropdown list should also have one set of identifiers without the three dashes option.
Screenshot
Editions dropdown
Work dropdown
Author dropdown
Stakeholders
@cdrini