Skip to content
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

Support null as a child element for ListItem #584

Merged
merged 3 commits into from
Jan 14, 2019

Conversation

gugu
Copy link
Contributor

@gugu gugu commented Jan 2, 2019

Last few years React supports null as a valid child (it just ignores this element). Used mostly for conditional statements. This pull request makes ListItem support nulls

@codecov-io
Copy link

codecov-io commented Jan 2, 2019

Codecov Report

Merging #584 into rc0.9.0 will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           rc0.9.0     #584      +/-   ##
===========================================
+ Coverage    94.36%   94.44%   +0.07%     
===========================================
  Files           61       61              
  Lines         2575     2575              
  Branches       376      376              
===========================================
+ Hits          2430     2432       +2     
+ Misses          51       50       -1     
+ Partials        94       93       -1
Impacted Files Coverage Δ
packages/list/ListItem.tsx 97.87% <100%> (+4.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f09bcf...0d56bb2. Read the comment docs.

@moog16
Copy link

moog16 commented Jan 9, 2019

@gugu could you please provide your use case? I see what you're saying, but there might be a better way of doing what you're trying to do.

Also this change requires a unit test. Thanks!

@gugu
Copy link
Contributor Author

gugu commented Jan 9, 2019

Here is an example:

return <AppListItem key={domain.id}>
    <DomainNameText primaryText={ domain.hostname } />
    { this.props.editPermission ? <DomainListActions>
        <AppIconButton onClick={this.handleDomainEdit(domain)} type="button"><MaterialIcon icon="edit" /></AppIconButton>
        <AppIconButton onClick={this.handleDeleteDomain(domain)} type="button"><MaterialIcon icon="delete" /></AppIconButton>
    </DomainListActions> : null} 
</AppListItem>;})

Here is how I solved the problem (creates unnecessary DOM node):

return <AppListItem key={domain.id}>
    <DomainNameText primaryText={ domain.hostname } />
    { this.props.editPermission ? <DomainListActions>
        <AppIconButton onClick={this.handleDomainEdit(domain)} type="button"><MaterialIcon icon="edit" /></AppIconButton>
        <AppIconButton onClick={this.handleDeleteDomain(domain)} type="button"><MaterialIcon icon="delete" /></AppIconButton>
    </DomainListActions> : <div />} 
</AppListItem>;})

@moog16
Copy link

moog16 commented Jan 9, 2019

I see...ya that seems to be the easiest way. I was thought the best way to render list items were iterating over an array...which you could use .filter to hide a ListItem. But in the case you have one list item this makes the most sense.

Can you add a unit test for this case? I will approve it then. Thanks for providing the example.

@moog16 moog16 changed the base branch from master to rc0.9.0 January 10, 2019 18:27
@gugu
Copy link
Contributor Author

gugu commented Jan 13, 2019

Done

@moog16
Copy link

moog16 commented Jan 14, 2019

@gugu thanks! I'm reviewing right now. Could you also sign the PR?

@moog16
Copy link

moog16 commented Jan 14, 2019

Tests are passing. Once signed we can merge it in :)

@gugu
Copy link
Contributor Author

gugu commented Jan 14, 2019

One moment, I slightly screwed up your merge with git push -f, so I did it by myself

@moog16
Copy link

moog16 commented Jan 14, 2019

Ah thanks...Although I just need you to comment "I signed it" por favor

@gugu
Copy link
Contributor Author

gugu commented Jan 14, 2019

I signed it

@moog16 moog16 merged commit b0e216e into material-components:rc0.9.0 Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants