Feature/1267 refactor bookmarks plugin #1270
Conversation
Refactor from class component to functional component.
Refactor from class component to functional component.
Refactor from class component to functional component.
Remove setMap and setApp as the map and app state are not meant to be updated.
Fix a bug with state handling and remove an unused `useEffect` import.
|
Nice! It looks promising. I will check more and test next week. Had a quick check regarding storage format and it's indeed the parameters that are stored, including map name. Bookmarks are per domain not per map.
|
There was a problem hiding this comment.
I've just given it a quick look. Will give it a deeper dive next week. However, i have some questions right away:
- Why are you rewriting and incorporating react functionality into
BookmarksModel.js? This was just a regular js class, not a react class component (should/does not have to not be rewritten). Also i really don't think it a good idea with a react component(?) returning an object...? Or is this some kind of custom hook with bad naming? (Custom hooks should have theusekeyword in front). I might just be too tired and missing something obvious. My main issue with this is that all our plugins are written with components along with one (or more) "model" (a regular js class). I don't see the point diverging from this "standard". - You "initialize" the model by writing
const bookmarksModel = BookmarksModel({ app, map });. This code will run on every re-render ofBookmarks.jswhich is really unnecessary. You can look at the dummy-example where i thoroughly explain how this should be done. updateCustomPropis unused and should be removed.
|
I agree with Hallbergs comment. BookmarkModel.js should be kept as ordinary class, no reason to bloat it with react. I will look again and test when this is fixed. |
There was a problem hiding this comment.
I'm not sure if you're done? I checked the code again, and found some issues.
First of all, make sure we're compiling without warnings. See image below:
The warnings stems from the fact that you've added stuff (from Dummy?) that are unnecessary, For example the pluginShown state was added in Dummy to show how to handle events from closing and opening the plugin window.
It also seems like you are adding a localObserver without actually using it. Not sure why.
Create file containing all necessary constant values for the Bookmarks plugin.
Reduced Local storage values to only include `settings`. Removed unused values `name`, `sortOrder`, and `favorite`.
Remove unused states, functions and `React.useEffect` hook from code.
Implement support for legacy bookmarks using `LocalStorageHelper` for backward compatibility.
Update code to handle and render new object structure with updated properties.
Add every legacy bookmark to corresponding map-specific storage using the `m` parameter.
linusfj
dismissed
jacobwod’s stale review
Pull request approved, ready for merge.
Closes #1267.