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

feat(Dropdown): lazyLoad prop for menu items #1918

Merged
merged 6 commits into from
Jun 26, 2018

Conversation

dsirnk
Copy link
Contributor

@dsirnk dsirnk commented Aug 1, 2017

Lazy-loading options will be very useful and efficient when there are many options.
We could eventually bind it to a prop like 'lazyLoad={true}'.

@codecov-io
Copy link

codecov-io commented Aug 1, 2017

Codecov Report

Merging #1918 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1918      +/-   ##
==========================================
+ Coverage   99.74%   99.78%   +0.04%     
==========================================
  Files         154      161       +7     
  Lines        2712     2765      +53     
==========================================
+ Hits         2705     2759      +54     
+ Misses          7        6       -1
Impacted Files Coverage Δ
src/modules/Dropdown/Dropdown.js 100% <100%> (ø) ⬆️
src/collections/Form/FormInput.js 100% <0%> (ø) ⬆️
src/collections/Table/TableBody.js 100% <0%> (ø) ⬆️
src/views/Card/CardHeader.js 100% <0%> (ø) ⬆️
src/modules/Search/SearchResult.js 100% <0%> (ø) ⬆️
src/elements/Icon/IconGroup.js 100% <0%> (ø) ⬆️
src/elements/Label/LabelGroup.js 100% <0%> (ø) ⬆️
src/collections/Menu/MenuHeader.js 100% <0%> (ø) ⬆️
src/modules/Popup/PopupContent.js 100% <0%> (ø) ⬆️
src/addons/TextArea/TextArea.js 100% <0%> (ø) ⬆️
... and 151 more

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 9af142e...731f4bb. Read the comment docs.

@dsirnk
Copy link
Contributor Author

dsirnk commented Aug 3, 2017

Sorry this is my first PR ever.

Anyone know what the issue is here?
I made the change so that the dropdown only populates with options when it's open (lazy loading).
I made respective changes to the unit tests to account for the change. And ensuring that all the tests pass.

However, I don't know what codecov is and why the codecov/patch and codecov/project are failing.

@levithomason
Copy link
Member

Codecov seems to be broken. It measures code coverage in our project. They changed how they calculate that recently and it hasn't recovered. You can safely ignore it for now.

.should.have.prop('active', true)
// `first().simulate('click')` closes the Dropdown so can't verify `prop('active', true)`
// TODO: Find a way to verify `first()` has `prop('active', true)`
// .should.have.prop('active', true)
Copy link
Member

Choose a reason for hiding this comment

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

I would think re-opening the dropdown would retain the active item. You could simply try something like (untested):

wrapper
  .find('DropdownItem')
  .first()
  .simulate('click')

// re-open the dropdown
wrapper.setProps({ open: true })

wrapper
  .find('DropdownItem')
  .first()
  .should.have.prop('active', true)

Copy link
Member

@levithomason levithomason left a comment

Choose a reason for hiding this comment

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

Would like to resolve #1918 (comment) before merge.

@stale
Copy link

stale bot commented Jun 14, 2018

There has been no activity in this thread for 90 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 90 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@stale stale bot added stale and removed stale labels Jun 14, 2018
@levithomason levithomason force-pushed the feat/dropdown/lazy-load branch from d32dc07 to 1001f0e Compare June 19, 2018 12:55
@levithomason
Copy link
Member

I've rebased this to master, moved the logic to a lazyLoad prop, and cleaned up tests. This can be merged after tests pass and a review is done.

@levithomason levithomason changed the title feat(Dropdown): lazy load options feat(Dropdown): lazyLoad prop for menu items Jun 26, 2018
@levithomason levithomason merged commit 95429e6 into Semantic-Org:master Jun 26, 2018
@welcome
Copy link

welcome bot commented Jun 26, 2018

Congrats on merging your first pull request! 🎉🎉🎉

robot victory dance

@levithomason
Copy link
Member

Released in semantic-ui-react@0.81.2.

@hubertguillemain
Copy link

I just created a bug (#2975) , as I don't see this working as I would expect.
But maybe I'm doing it wrong?
If so, can you please explain and add a sample usage to the doc site ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants