-
Notifications
You must be signed in to change notification settings - Fork 3
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
Implemented: NetSuite Integration Management UI(#37) #38
base: main
Are you sure you want to change the base?
Conversation
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.
See comments
src/views/NetSuite.vue
Outdated
</script> | ||
|
||
<style scoped> | ||
main { |
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 use main element here? we can simply just add ion padding to ion content right? @R-Sourabh
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.
Yeah, sure sir.
src/views/NetSuite.vue
Outdated
<ion-icon slot="end" :icon="chevronForwardOutline"/> | ||
</ion-item> | ||
</ion-card> | ||
<ion-card> |
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.
we shouldn't use ion-card here. Like in Figma, directly use ion item with outline and border radius. We should make this a global style in our app themes. We use this item pattern in the fulfillment app and I expect to use it again. @R-Sourabh
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 misunderstood that sir, I'll change it.
src/views/NetSuite.vue
Outdated
|
||
<div class="ion-margin-top"> | ||
<ion-text>Configuration</ion-text> | ||
<section> |
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 this UI patter in itself will be, or has been frequently reused. Lets make a DXP component where you can pass in:
list of item labels and p tags in labels. You can also pass in the action icon that will be displayed on each of the items' end slots.
something like:
title: section-name
itemEndIcon: icon-name
items: [{primary label, secondary label}]
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.
Sure, I'll look into this.
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.
Some more feedback
src/views/NetSuite.vue
Outdated
} | ||
|
||
.item-box::part(native) { | ||
border-radius: 8px; |
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 not just use the ion item custom property for border radius?
https://ionicframework.com/docs/api/item#css-custom-properties-1
--border-radius
grid-template-columns: repeat(auto-fill, minmax(300px, 1fr)); | ||
gap: 10px; |
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.
use spacer variables here instead of hard coded value. I know you are recreating the default margin of ion card, but if you're replacing, it is recommended to use premade spacer.
…and added the NetSuite service (hotwax#37)
… services for integration & shopify type mappings(hotwax#37)
…r action/service name(hotwax#37)
…t store is configured & add action to clear the selected product store after logout(hotwax#37)
…hanges in NetSuite modals, and added a check to prevent fetching the virtual facility on the departments page (hotwax#37)
…evel modals, and updated the logic for fetching mappings (hotwax#37)
Related Issues
#37
Short Description and Why It's Useful
Screenshots of Visual Changes before/after (If There Are Any)
Contribution and Currently Important Rules Acceptance