Skip to content
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

[feature] Always show a wishlist name and do not show visibility toggle if only one list #3184

Merged
merged 6 commits into from
May 26, 2021

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented May 20, 2021

Description

  • Added placeholder text for display if there is no wishlist name configured.
  • Show or hide visibility toggle based on number of wishlists belonging to a user.

Related Issue

Closes PWA-1742.

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Verify a wishlist without a name at least has the default "Wish List" name text.
  2. In the user's wishlist page, verify that the visibility toggle does not appear if there is only one list.

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have added translations for new strings, if necessary.
  • I have updated the documentation accordingly, if necessary.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented May 20, 2021

Messages
📖

Associated JIRA tickets: PWA-1742.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against d9b1184

@sirugh sirugh added version: Patch This changeset includes backwards compatible bug fixes. version: Minor This changeset includes functionality added in a backwards compatible manner. and removed version: Patch This changeset includes backwards compatible bug fixes. labels May 20, 2021
Signed-off-by: sirugh <rugh@adobe.com>
revanth0212
revanth0212 previously approved these changes May 25, 2021
Copy link
Contributor

@revanth0212 revanth0212 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than a question I had about using a talon instead, the code looks fine. Nice work buddy.

<Wishlist
key={wishlist.id}
data={wishlist}
shouldRenderVisibilityToggle={wishlists.length > 1}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it merit if we create and send it over from the talon instead? By doing so we are opening up the component to have custom definitions for shouldRenderVisibilityToggle. In the default case, it will be wishlists.length > 1. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, sounds good.

@revanth0212
Copy link
Contributor

There are some test failures. Check it out dude.

Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
@dpatil-magento dpatil-magento merged commit 8e35266 into develop May 26, 2021
@michaelyu0123 michaelyu0123 deleted the pwa-1742/single-wishlist-no-collapse branch February 1, 2022 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:peregrine pkg:venia-ui Progress: done version: Minor This changeset includes functionality added in a backwards compatible manner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants