Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(List): reduce number of rendered DOM nodes#1218

Merged
layershifter merged 5 commits intomasterfrom
chore/list-layout
Apr 15, 2019
Merged

fix(List): reduce number of rendered DOM nodes#1218
layershifter merged 5 commits intomasterfrom
chore/list-layout

Conversation

@layershifter
Copy link
Member

@layershifter layershifter commented Apr 12, 2019

Fixes #1029.

  • no visual regressions ✔️
  • no breaking changes ✔️

With these changes we will render Flex and corresponding div only when we will really need them.

@codecov
Copy link

codecov bot commented Apr 12, 2019

Codecov Report

Merging #1218 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1218      +/-   ##
==========================================
+ Coverage   82.56%   82.57%   +0.01%     
==========================================
  Files         752      752              
  Lines        8859     8868       +9     
  Branches     1178     1186       +8     
==========================================
+ Hits         7314     7323       +9     
  Misses       1530     1530              
  Partials       15       15
Impacted Files Coverage Δ
packages/react/src/components/List/ListItem.tsx 100% <100%> (ø) ⬆️

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 995b954...24c0823. Read the comment docs.

@layershifter layershifter changed the title [WIP] fix(List): reduce number of rendered DOM nodes fix(List): reduce number of rendered DOM nodes Apr 12, 2019
@layershifter layershifter added 🧰 fix Introduces fix for broken behavior. 🚀 ready for review labels Apr 12, 2019
Copy link
Collaborator

@bmdalex bmdalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good stuff 👍 definitely an improvement
Screenshot 2019-04-12 at 16 56 38

CHANGELOG.md Outdated
- Fix routing for accessibility documentation @sophieH29 ([#1208](https://github.com/stardust-ui/react/pull/1208))
- Fix `content` prop type in `Dialog` @layershifter ([#1212](https://github.com/stardust-ui/react/pull/1212))
- Add `keyboard` up & down key controls for the `Tree` component @priyankar205 ([#1219]https://github.com/stardust-ui/react/pull/1219)
- Add `keyboard` up & down key controls for the `Tree` component @priyankar205 [#1219](https://github.com/stardust-ui/react/pull/1219)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are still missing one pair of parentheses 🤣

@layershifter layershifter merged commit d8dd9a6 into master Apr 15, 2019
@delete-merged-branch delete-merged-branch bot deleted the chore/list-layout branch April 15, 2019 08:12
{contentMediaElement}
</Flex>
<Flex column={hasBothParts} className={ListItem.slotClassNames.main} styles={styles.main}>
{this.wrapWithFlex(headerPart, hasBothParts)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for commenting after this was merged. Why we have confition for adding Flex if the content and header parts exists, when the Flex we are using are adding gap between the element + mediaElement. With this we have regression (the gap between header + headerMedia and content + contentMedia are incorrect if the header and content are not applied both..).

Previous:
image

Current:
image

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🧰 fix Introduces fix for broken behavior. 🚀 ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RFC: List item DOM optimization

4 participants