-
Notifications
You must be signed in to change notification settings - Fork 685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add to Wishlist from PDP #3048
Add to Wishlist from PDP #3048
Conversation
|
…ove from favorites ux Signed-off-by: sirugh <rugh@adobe.com>
packages/venia-ui/lib/components/Wishlist/WishlistDialog/NewWishlistForm/newWishlistForm.js
Outdated
Show resolved
Hide resolved
<button onClick={handleNewListClick} type="button"> | ||
{createButtonText} | ||
</button> | ||
<Relevant when={() => !!isOpen}>{maybeForm}</Relevant> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the right place to use Relevant
. Should we memoize the when
function here?
const shouldRenderForm = useCallback(
() => !!isOpen,
[isOpen]
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
import NewWishlistForm from './NewWishlistForm/newWishlistForm'; | ||
import FormError from '../../FormError'; | ||
|
||
const WishlistEntry = props => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd put this in its own file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
option.attribute_id, | ||
option.values | ||
]) | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of useMemo
to convert this array to a map. 👍
Optionally, a small optimization would be to skip the intermediate array (faster, less memory):
const attributeIdToValuesMap = useMemo(
() => {
const map = new Map()
for (const { attribute_id, values } of options) {
map.set(attribute_id, values)
}
},
[options]
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
@sirugh Since there has been some 'compromise' on behavior for "add to Favs" in that while a shopper may tap to add to Favs, they cannot tap again to remove (instead go to list to manage list items). I would propose the following revisions to the UI.
Additionally, I think it is somewhat better if when a shopper taps "Create New List" but then decides to Cancel (and not create a new list) that only the Create New List form closes and not the entire drawer/modal. |
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look great!
There were some areas where the merge overlapped with the recently merged
#3052 , so we should give a quick smoke test of the overlap of these two features.
@sirugh Everything Looks great other than Link UI on Virtual Product PDP and Safari browser. |
…enter. Signed-off-by: sirugh <rugh@adobe.com>
Discussed with Dev on a call - the safari issue with the icon appearing above the text seems to be some issue with Safari 13 as it does not happen on my local Safari 14. Inspecting further it seems the span for the icon is spread to the max width of the button above, whereas in Safari 14 the enclosing icon span just matches the icon width. Maybe a grid issue they fixed? |
QA Approved |
Signed-off-by: sirugh <rugh@adobe.com>
fd05d95
to
5620add
Compare
Description
Add a product to your wishlist(s) from PDP for both CE and EE.
Related Issue
Closes PWA-1267.
Acceptance
Verification Stakeholders
Specification
Verification Steps
CE
EE
Screenshots / Screen Captures (if appropriate)
Checklist