- 
                Notifications
    
You must be signed in to change notification settings  - Fork 65
 
Update template to use RR7 app template and Polaris web components #639
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
Conversation
ba457d0    to
    037a781      
    Compare
  
    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.
@davejcameron please can you or team review the UI/UX changes?
| <link | ||
| rel="stylesheet" | ||
| href="https://cdn.shopify.com/static/fonts/inter/v4/styles.css" | ||
| /> | 
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.
Why this addition?
| } | ||
| 
               | 
          ||
| li { | ||
| .list > li { | 
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.
Why change from element selectors to classes? I get why it's better, but just want to understand if thisis related/unrelated to RR. I think unrelated?
| 
           The weird part is the button on the title bar with Payment customizations, I don't think that'd be what we want.  | 
    
| Text, | ||
| VerticalStack, | ||
| } from "@shopify/polaris"; | ||
| import { TitleBar } from "@shopify/app-bridge-react"; | 
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.
Should we use https://shopify.dev/docs/api/app-bridge-library/web-components/ui-title-bar instead then at least we are all web components
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.
Yes, I actually just brought this up to Richard. I think it makes sense for us to use all web components. @byrichardpowell if you are aligned I will update here and in the template.
| 
               | 
          ||
| const responseJson = await response.json(); | ||
| const metafield = | ||
| responseJson.data.paymentCustomization?.metafield?.value && | 
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.
Unrelated to your work, but I wish this used jsonValue rather than .value
037a781    to
    a16b5ee      
    Compare
  
    
          
 @davejcameron I updated it to use the contextual save bar, with bread crumbs. Which I more inline with what is outlined in the patterns doc.  
       | 
    
13fb002    to
    23a776c      
    Compare
  
    f6c83d4    to
    e571dcb      
    Compare
  
    
What is this PR doing
Functions UI Before
Functions UI After