BB-874: feat(editor): add recently used entities to dropdowns#1207
BB-874: feat(editor): add recently used entities to dropdowns#1207atharvsp02 wants to merge 6 commits intometabrainz:masterfrom
Conversation
511be94 to
cc59ea3
Compare
cc59ea3 to
946e482
Compare
|
@MonkeyDo |
MonkeyDo
left a comment
There was a problem hiding this comment.
Hi @atharvsp02 sorry for the delay!
Functionally, the PR works great, does exaclty what it says on the tin, nice work!
Code-wise, I do have a few suggestions to try and simplify the code a bit and remove redundancies.
| static entityTypeMappings = { | ||
| // Saving | ||
| Area: 'areas', | ||
| Author: 'authors', | ||
| Edition: 'editions', | ||
| EditionGroup: 'editiongroups', | ||
| Publisher: 'publishers', | ||
| Series: 'series', | ||
| Work: 'works', | ||
| // Loading | ||
| areas: 'Area', | ||
| authors: 'Author', | ||
| editiongroups: 'EditionGroup', | ||
| editions: 'Edition', | ||
| publishers: 'Publisher', | ||
| series: 'Series', | ||
| works: 'Work' | ||
| }; |
There was a problem hiding this comment.
I'm not sure this is necessary, I have an inkling that it is overcomplicating things.
IMO you can save to localstorage using the singular key and use the type prop directly to and avoid a lot of this.
If I had to go all the way, I would do the following, although it might be best kept for a separate PR considering it requires making this file a typescript file, which will cause more changes than necessary.
We define the entity type strings which you can import form our exported types:
import {ENTITY_TYPES} from 'bookbrainz-data/lib/types/entity';
Then you need to add Area to it, as it is not part of the entity types:
const entityTypeStrings = [...ENTITY_TYPES, 'Area'] as const;
The addition of as const allows us to use the array of strings as a precise type for the recentlyUsedEntityType prop, with type checking:
type SearchFieldEntityTypes = typeof typestring[number];
Then when used as a type you get autocompletion and type checking:
There was a problem hiding this comment.
Yeah you're right, that was overcomplicating it. Using singular types directly is simpler, so removed the mapping now!
Should I do the TypeScript conversion with ENTITY_TYPES in a separate PR?
There was a problem hiding this comment.
I would usually say that it's probably best as a separate PR, as it is going to be a lot of changes which makes it harder to review.
But I think in this case it makes sense to do it in the same PR, and you're already modifying most of the important lines of that file.
|
|
||
| async fetchOptions(query) { | ||
| const recentOptions = this.getRecentlyUsedOptions(); | ||
| const uniqueRecentOptions = _.uniqBy(recentOptions, 'id').slice(0, 10); |
There was a problem hiding this comment.
This array is already made unique in getRecentlyUsedOptions so you should be able to remove this line.
There was a problem hiding this comment.
Right, didn't notice that. Since getRecentlyUsedOptions() already handles uniqueness, the extra check here wasn't needed. Removed it!
| const storageKey = EntitySearchFieldOption.entityTypeMappings[entityOption.type] || `${entityOption.type.toLowerCase()}s`; | ||
| RecentlyUsed.addItem(storageKey, { | ||
| id: entityOption.id, | ||
| name: entityOption.text | ||
| }); |
There was a problem hiding this comment.
Here is a good example: I would just use the entity type as the stotageKey, I don't think it's that important to have it lowercased or pluralized if it removes the need for a mapping and for the recentlyUsedEntityType prop, since those localstorage keys are never shown to end users. Simplifies things quite a bit I think.
| const storageKey = EntitySearchFieldOption.entityTypeMappings[entityOption.type] || `${entityOption.type.toLowerCase()}s`; | |
| RecentlyUsed.addItem(storageKey, { | |
| id: entityOption.id, | |
| name: entityOption.text | |
| }); | |
| RecentlyUsed.addItem(entityOption.type, { | |
| id: entityOption.id, | |
| name: entityOption.text | |
| }); |
There was a problem hiding this comment.
Done, using the entity type directly now.
| onAddSeriesItem: (data) => dispatch(addSeriesItem(data, data.rowID)), | ||
| onChange: (value:any) => dispatch(addWork(value)), | ||
| onChange: (value:any) => { | ||
| if (value && value.id && value.text) { |
There was a problem hiding this comment.
I think you might be able to make this into a more generic function (exported from recently-used.ts for example) you can reuse in those various cases, whenever (like here) you have a value with type, you can call i.e. addItemToRecentlyUsed(value), which will check for the presence of type, id and text.
You can reuse it below for series, and in other files where you do basically the same thing for various entity types.
There was a problem hiding this comment.
Thanks for the suggestion. Created the helper function and updated the relevant files.
| onAddSeriesItem, onSubmitWork, resetSeries, bulkAddSeriesItems, ...rest}:ContentTabProps) { | ||
| const [isChecked, setIsChecked] = React.useState(true); | ||
| const [copyToSeries, setCopyToSeries] = React.useState(false); | ||
| const [onChangeRefresh, setOnChangeRefresh] = React.useState(0); |
There was a problem hiding this comment.
This looks like a workaround, can you talk me through the issue that you are solving with this?
I can see it is used as a key for the work select component, but I don't understand why it is needed.
Let's get to the source of the issue together and see if we need to properly fix something to try and remove the workaround.
There was a problem hiding this comment.
The dropdown caches the "Recently Used" list when it loads. Without this workaround, after selecting a work, we have to refresh the whole page for it to show up in the "Recently Used" section.
The key change forces the dropdown to reload and show the updated list right away.
Is there a better way to handle this?
There was a problem hiding this comment.
Is this always the case, or is it only for the language select?
I ask because LanguageSelect has cacheOptions set to true but not other selects.
bookbrainz-site/src/client/entity-editor/common/language-field.tsx
Lines 129 to 130 in a16d1db
In any case, I think it's probably fine to keep the code as-is if it solves the issue
There was a problem hiding this comment.
Is this always the case, or is it only for the language select? I ask because LanguageSelect has
cacheOptionsset to true but not other selects.bookbrainz-site/src/client/entity-editor/common/language-field.tsx
Lines 129 to 130 in a16d1db
In any case, I think it's probably fine to keep the code as-is if it solves the issue
I noticed this specifically in the Work dropdown on content tab of Unified Form. Since the 'Recently Used' list is loaded at mount into defaultOptions and then cached, it doesn't refresh automatically when localStorage changes. The key workaround helps force that refresh.
I will keep the code as-is then since it resolves the issue
MonkeyDo
left a comment
There was a problem hiding this comment.
Hello @atharvsp02 I'm not sure if you were waiting for a response but I don't see the changes you mentioned in your last few messages, not sure if you didn't push them.
| onAddSeriesItem, onSubmitWork, resetSeries, bulkAddSeriesItems, ...rest}:ContentTabProps) { | ||
| const [isChecked, setIsChecked] = React.useState(true); | ||
| const [copyToSeries, setCopyToSeries] = React.useState(false); | ||
| const [onChangeRefresh, setOnChangeRefresh] = React.useState(0); |
There was a problem hiding this comment.
Is this always the case, or is it only for the language select?
I ask because LanguageSelect has cacheOptions set to true but not other selects.
bookbrainz-site/src/client/entity-editor/common/language-field.tsx
Lines 129 to 130 in a16d1db
In any case, I think it's probably fine to keep the code as-is if it solves the issue
Hi, |
827d51c to
5c2c86b
Compare
|
@MonkeyDo I made the suggested changes, please review it whenever possible |
5c2c86b to
c039834
Compare
Thanks for the changes! CI issues are unrelated, I'm working on a big update that will also fix it. |
Problem
Entity selection dropdowns were missing recently used items, requiring editors to repeatedly search for the same entity.his created workflow friction for editors who frequently work with the same entities.
Solution
Extended the recently used feature to all entity dropdowns:
RecentlyUsedservice for localStorage managementAreas of Impact
Components Modified:
EntitySearchFieldOption- Core dropdown componentContentTab- Works and Series dropdownsCoverTab- Publisher and Author credit dropdownsEditionSection- Edition group, Publisher and Author credit dropdownsPublisherSection- Area dropdownAuthorSection- Area dropdownRelationshipModal- Single and Multi-entity relationship dropdownUser Experience:
Screenshots