-
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
} | ||
|
||
.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.
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