-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat(quantic): Fix the styling of the quantic sort component when inside refine modal #4652
base: master
Are you sure you want to change the base?
Conversation
Pull Request ReportPR Title❌ Title should follow the conventional commit spec: Example: Live demo linksBundle Size
SSR Progress
Detailed logssearch : buildInteractiveResultsearch : buildInteractiveInstantResult search : buildInteractiveRecentResult search : buildInteractiveCitation search : buildGeneratedAnswer recommendation : missing SSR support case-assist : missing SSR support insight : missing SSR support commerce : missing SSR support |
As an initial comment, the variant values need to be something generic and reflect the changes that would occur on the component it self and not where the component would be used. that's why the variant value A better used values could be |
6920088
to
8920c9a
Compare
Good point, changed it to "wide" since it takes the whole width |
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 suggestions on the code structure, but overall it seems to be working fine
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.html
Show resolved
Hide resolved
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.
Changes required for better scalability of the sort component that requires avoid mixing the size of the sort input with the label.
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.js
Outdated
Show resolved
Hide resolved
...uantic/force-app/main/default/lwc/quanticRefineModalContent/templates/dynamicNavigation.html
Outdated
Show resolved
Hide resolved
a157bb3
to
d09c9ca
Compare
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.html
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/__tests__/quanticSort.test.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/__tests__/quanticSort.test.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/__tests__/quanticSort.test.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/__tests__/quanticSort.test.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/__tests__/quanticSort.test.js
Outdated
Show resolved
Hide resolved
5da7879
to
890cf0f
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.
Nice job 👌 The final result blends well in the refine modal
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.
Slight changes and some unused code I think?
* @api | ||
* @type {'default'|'wide'} | ||
*/ | ||
@api variant = 'default'; |
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.
Can you do this instead?
@api variant = 'default'; | |
@api variant = SORT_VARIANTS.DEFAULT; |
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.
nah I tried, its giving me a type error:
either I set the @type to string, or leave it as is and set the default value to 'default'
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.js
Outdated
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticSort/quanticSort.js
Outdated
Show resolved
Hide resolved
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.
Perfect!
SFINT-4897
ISSUE:
The styling of the quanticSort component does not match what we do for the filters and Atomic's. Changing this styling is not as simple however. The
lightning-combobox
does not expose selectors to change its styling such as height, border, etc.SOLUTION PROPOSED:
should construct itself without throwing
LIMITATIONS:
POTENTIAL ALTERNATIVES:
lightning-combobox
for accessibility and so on. It make the quanticSort component a bit more complex than it needs to be.BEFORE:
AFTER:
Screen.Recording.2024-11-08.at.1.57.34.PM.mov
TESTS: