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

Histogram slider component UI #100

Closed
wants to merge 10 commits into from

Conversation

zlatanpham
Copy link
Contributor

Addresses the UI part of #98. Examples of usage can be found by running docs.

Copy link
Member

@benhinchley benhinchley left a comment

Choose a reason for hiding this comment

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

Overall looking pretty good, request a couple of changes.

src/components/HistogramSlider/Histogram/Histogram.tsx Outdated Show resolved Hide resolved
src/components/HistogramSlider/Histogram/Histogram.tsx Outdated Show resolved Hide resolved
src/components/HistogramSlider/HistogramSlider.tsx Outdated Show resolved Hide resolved
src/components/HistogramSlider/HistogramSlider.tsx Outdated Show resolved Hide resolved
src/components/HistogramSlider/RangeSlider/RangeSlider.tsx Outdated Show resolved Hide resolved
src/components/HistogramSlider/RangeSlider/RangeSlider.tsx Outdated Show resolved Hide resolved
@zlatanpham
Copy link
Contributor Author

Hey @benhinchley I've made the update. Can you take a look?
Btw, thanks for pointing it out. I should have known the deprecation earlier :)

@benhinchley
Copy link
Member

benhinchley commented Feb 13, 2019

@zlatanpham This is looking good, apologies for not attending to this sooner, I am currently battling with the documentation tool so I can get this running locally. Also I've just noticed there a couple of file conflicts, can you rebase this branch against current master and fix those.

@zlatanpham zlatanpham force-pushed the histogram-slider-component branch from bcd2471 to cb7b7e9 Compare February 15, 2019 09:18
@zlatanpham
Copy link
Contributor Author

@benhinchley No problem! let me know if you want me take a look at the document problem too.

@sampotts
Copy link
Contributor

sampotts commented Apr 8, 2020

Hey @benhinchley & @zlatanpham , I'm just looking at this issue that's been raised recently and it looks like some of the work in this PR could be used to solve the issue. Any ideas?

@benhinchley
Copy link
Member

Hey @benhinchley & @zlatanpham , I'm just looking at this issue that's been raised recently and it looks like some of the work in this PR could be used to solve the issue. Any ideas?

Yeah @sampotts This can be used, I for the life of me cannot remember why I didn't merge this in the end.

@zlatanpham zlatanpham closed this Dec 14, 2020
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.

3 participants