EZP-29266: Content item seems to be always bookmarked #509
Conversation
… adds propser CSS class on bookmark form wrapper; small adjustments in CSS styles were added.
tischsoic
changed the title
| const bookmarkCheckedClass = 'ez-add-to-bookmarks--checked'; | ||
|
|
||
| if (bookmarked) { | ||
| wrapperClassList.add(bookmarkCheckedClass); |
There was a problem hiding this comment.
Not an expert, but I believe there is toggle method
@sunpietro
There was a problem hiding this comment.
This function also manages CSS class on event: ez-bookmark-change.
I assume that this event with bookmarked property set to true can occur even if the content is already bookmarked.
This is why I decided not to use toggle function.
There was a problem hiding this comment.
You can do it like this wrapper.classList.toggle('ez-add-to-bookmarks--checked', isBookmarked);. See the reference: https://developer.mozilla.org/en-US/docs/Web/API/Element/classList
| @@ -15,14 +16,31 @@ | |||
| const isCurrentLocation = (locationId) => { | |||
| return parseInt(locationId, 10) === currentLocationId; | |||
| }; | |||
| const setBookmarkWrapperCheckedClass = (bookmarked) => { | |||
| const wrapperClassList = document.querySelector(SELECTOR_BOOKMARK_WRAPPER).classList; | |||
There was a problem hiding this comment.
Should be const wrapper = document.querySelector(SELECTOR_BOOKMARK_WRAPPER);
| @@ -15,14 +16,31 @@ | |||
| const isCurrentLocation = (locationId) => { | |||
| return parseInt(locationId, 10) === currentLocationId; | |||
| }; | |||
| const setBookmarkWrapperCheckedClass = (bookmarked) => { | |||
| const bookmarkCheckedClass = 'ez-add-to-bookmarks--checked'; | ||
|
|
||
| if (bookmarked) { | ||
| wrapperClassList.add(bookmarkCheckedClass); |
There was a problem hiding this comment.
You can do it like this wrapper.classList.toggle('ez-add-to-bookmarks--checked', isBookmarked);. See the reference: https://developer.mozilla.org/en-US/docs/Web/API/Element/classList
| const updateBookmarkForm = (event) => { | ||
| const { bookmarked, locationId } = event.detail; | ||
|
|
||
| if (isCurrentLocation(locationId)) { | ||
| updateBookmarkCheckbox(bookmarked); | ||
| setBookmarkWrapperCheckedClass(bookmarked); |
There was a problem hiding this comment.
I don't like the function name. I see it more like this: toggleBookmarkCheckedState
There was a problem hiding this comment.
Maybe, toggleBookmarkIconState?
| } | ||
| }; | ||
| const updateBookmarkWrapperClass = (event) => { |
There was a problem hiding this comment.
updateBookmarkCheckedState
There was a problem hiding this comment.
I think that updateBookmarkCheckedState suggests that the input state is being updated, when it is not.
There was a problem hiding this comment.
Maybe, updateBookmarkIconState?
| @@ -15,14 +16,31 @@ | |||
| const isCurrentLocation = (locationId) => { | |||
| return parseInt(locationId, 10) === currentLocationId; | |||
| }; | |||
| const setBookmarkWrapperCheckedClass = (bookmarked) => { | |||
| const wrapperClassList = document.querySelector(SELECTOR_BOOKMARK_WRAPPER).classList; | |||
| const bookmarkCheckedClass = 'ez-add-to-bookmarks--checked'; | |||
There was a problem hiding this comment.
This should be at the top CLASS_BOOKMARK_CHECKED.
|
I don't where the markup was fixed to use bootstrap one? |
|
@dew326 @sunpietro are you OK with last changes? |
| @@ -4,6 +4,8 @@ | |||
| const SELECTOR_BOOKMARK_LOCATION_INPUT = '#location_update_bookmark_location'; | |||
| const SELECTOR_BOOKMARK_WRAPPER = '.ez-add-to-bookmarks'; | |||
|
|
|||
| const CLASS_BOOKMARK_CHECKED = 'ez-add-to-bookmarks--checked'; | |||
There was a problem hiding this comment.
Please, no extra lines before this const
There was a problem hiding this comment.
You're using this CSS class once. There's no need to store it in a variable.
There was a problem hiding this comment.
@dew326 told me in above comment to store it in CLASS_BOOKMARK_CHECKED. :P
There was a problem hiding this comment.
Yes, because in the previous approach you were using it in 2 places in the code. Now it's used only once.
There was a problem hiding this comment.
It was when you were using it twice. There is no need to cache string when you use it once but it's also not a mistake, so it can stay. :)
Co-authored-by: katarzynazawada <>
Fixes the bug concerning a bookmark star on a location page. The content was always bookmarked, even if it wasn't.
Checklist:
$ composer fix-cs)