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

My Account - Create wishlist UI #2766

Merged
merged 23 commits into from
Oct 14, 2020
Merged

Conversation

revanth0212
Copy link
Contributor

@revanth0212 revanth0212 commented Oct 8, 2020

Description

Added the Create Wishlist logic.

Note: This feature is built ahead of the GQL support. Due to that, the create list mutation is not available and will be handled in PWA-989.

Note to @schensley: The mobile mocks had the buttons placed below the radio button group but since I am using the dialog component they are placed at the bottom. Do we have a standard that we would like to follow for all the mobile dialogs/drawers about where to place the buttons?

Related Issue

Closes PWA-629

Verification Stakeholders

@schensley

Verification Steps

  1. On the deployed app go to /wishlist.
  2. Click on the Create a List button.
  3. Fill out the dialog.

Screenshots / Screen Captures (if appropriate)

image

image

Checklist

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

@revanth0212 revanth0212 added pkg:peregrine version: Minor This changeset includes functionality added in a backwards compatible manner. pkg:venia-ui labels Oct 8, 2020
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Oct 8, 2020

Messages
📖

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

📖 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).
📖

Associated JIRA tickets: PWA-989.

Generated by 🚫 dangerJS against 49d67ab

@revanth0212 revanth0212 changed the title Revanth/my account create list My Account - Create wishlist UI Oct 8, 2020
@schensley
Copy link

@revanth0212 this work looks really good. UX approved.

import { useState, useCallback } from 'react';

/**
* @function
Copy link
Member

Choose a reason for hiding this comment

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

Why @function js doc ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of the new Docs initiate we have started for the code. I modeled this after @jcalcaben's previous PR. @jcalcaben is this as expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

@larsroettig this is used to help me auto-generate reference docs from the source code

@revanth0212 yes, this looks like the proper format

@@ -1,3 +1,52 @@
.root {
Copy link
Member

Choose a reason for hiding this comment

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

Empty root style ?

Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

Looks good, some suggestions to clean things up.

</Field>
<RadioGroup
classes={radioGroupClasses}
field="listtype"
Copy link
Contributor

Choose a reason for hiding this comment

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

Very odd, but I don't see listtype or sharing_code in the createWishlist mutation. If GraphQL does not support this field on creation, we should remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel that looks like a mistake. sharing_code seems to be part of the WishList interface though, hence I would say, let's have it in the UX till the WishList feature is available through GQL.

tjwiebell
tjwiebell previously approved these changes Oct 13, 2020
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

Please loop in the GraphQL team just in case we need to add list visibility as an input field; this looks perfect with current coverage though. Nice work 👍

@revanth0212
Copy link
Contributor Author

Please loop in the GraphQL team just in case we need to add list visibility as an input field; this looks perfect with current coverage though. Nice work 👍

Thanks, @tjwiebell. I have created an issue on the architecture board asking this question.

@dpatil-magento
Copy link
Contributor

@revanth0212 Please check below observations -

  1. Per mocks Private radio should be first and Public below it, but app shows opposite, expected?
  2. The entire Row is clickable, I think only the link should be clickable?

Image from Gyazo

@revanth0212
Copy link
Contributor Author

@revanth0212 Please check below observations -

  1. Per mocks Private radio should be first and Public below it, but app shows opposite, expected?
  2. The entire Row is clickable, I think only the link should be clickable?

Image from Gyazo

Thanks for the check Dev. I have addressed the order of the radio buttons. Regarding the link, I haven't changed it and I think it is as expected.

@dpatil-magento
Copy link
Contributor

@schensley Hope 2nd observations is expected?

@schensley
Copy link

@dpatil-magento @revanth0212
Regarding your question (2. above), ideally the entire area (row, button, container) should be clickable as it behaves in the demo. Provides for a much bigger target for the shopper.

@dpatil-magento dpatil-magento merged commit f207bba into develop Oct 14, 2020
@dpatil-magento dpatil-magento deleted the revanth/my_account_create_list branch October 14, 2020 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Event: Distributed CD India 2020 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.

8 participants