-
Notifications
You must be signed in to change notification settings - Fork 64
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
1709 list item structure #1710
base: master
Are you sure you want to change the base?
1709 list item structure #1710
Conversation
This pull request is being automatically deployed with Vercel (learn more). widget-test-docs – ./🔍 Inspect: https://vercel.com/dojo/widget-test-docs/EYyzdnsyvfPEyWRTmgbvrFNJmugs dojo.widgets – ./🔍 Inspect: https://vercel.com/dojo/dojo.widgets/74fyTdUpkKX6KQt2HSggavsNXwGL |
Codecov Report
@@ Coverage Diff @@
## master #1710 +/- ##
==========================================
- Coverage 90.07% 89.95% -0.12%
==========================================
Files 94 94
Lines 5047 5059 +12
Branches 1375 1377 +2
==========================================
+ Hits 4546 4551 +5
- Misses 249 254 +5
- Partials 252 254 +2
Continue to review full report at Codecov.
|
let listContents: RenderResult; | ||
|
||
if (isRenderResult(firstChild)) { | ||
listContents = firstChild; |
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.
Since it is only taking the first child, users of the widget will need to wrap the content in a <virtual>
element. Should this pass all the children so it could be used like the following?
<ListItem>
Text node
<span>sibling node</span>
</ListItem>
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.
good question
case 'small': | ||
paddingClass = themedCss.smallPadding; | ||
break; | ||
case 'medium': |
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 the options be 'small' and 'large'?
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.
could always add a large too, but that's fine by me
composes: mdc-list-item from '@material/list/dist/mdc.list.css'; | ||
} | ||
|
||
.smallPadding { |
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.
What does this padding default to?
padding-right: calc(var(--mdc-theme-grid-base) * 2); | ||
} | ||
|
||
.noPadding { |
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 avoid !important
by adding .root
to the selector?
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.
most likely, could be that material css is using a gnarly selector though
Type: bug / feature
The following has been addressed in the PR:
.dojorc
theme.variant()
is added to the root domnodetheme.compose
like thisDescription:
primary
span if typed children are used, this allows for full-width content to be passed.padding
property with specified sizes toListItem
to allow the padding to be easily reduced or increased.Resolves #1709