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

fix(Dropdown): use placeholder as default text #3586

Merged
merged 4 commits into from
Jul 7, 2019
Merged

fix(Dropdown): use placeholder as default text #3586

merged 4 commits into from
Jul 7, 2019

Conversation

eyas2014
Copy link
Contributor

@eyas2014 eyas2014 commented Apr 27, 2019

Fixes #3581.


We should always display some text in div.text because SUI does the same thing: https://semantic-ui.com/modules/dropdown.html Actually, this is also an answer why we don't have any unit tests for this behavior.

SUI has styles that will prevent visibility off div.text:

image

@welcome
Copy link

welcome bot commented Apr 27, 2019

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

  • Run yarn lint locally to catch formatting errors. This will fix some errors automatically, commit and push any changes.
  • Run yarn test locally to catch errors. This ensures all components still behave as they should.
  • Run yarn start to run the doc site locally and try a few pages, ensuring everything is in good working order.
  • Include tests when adding/changing behavior.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@codecov-io
Copy link

codecov-io commented Apr 27, 2019

Codecov Report

Merging #3586 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3586      +/-   ##
==========================================
- Coverage   99.83%   99.83%   -0.01%     
==========================================
  Files         174      174              
  Lines        3105     3103       -2     
==========================================
- Hits         3100     3098       -2     
  Misses          5        5
Impacted Files Coverage Δ
src/modules/Dropdown/Dropdown.js 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 f8e894c...1a15b4e. Read the comment docs.

@layershifter
Copy link
Member

Sorry for the late feedback. I want to check this change additionally because it was not covered by unit tests before 😮

@mattgd
Copy link

mattgd commented Jul 1, 2019

Hi @layershifter! Any word on this PR? Is there anything I might be able to do to help with testing to expedite this?

@layershifter layershifter changed the title Fix #3581 fix(Dropdown): use searchQuery as text Jul 7, 2019
@layershifter layershifter changed the title fix(Dropdown): use searchQuery as text fix(Dropdown): use placeholder as default text Jul 7, 2019
@layershifter
Copy link
Member

@mattgd @eyas2014 thank you for contributions and follow ups 👍

I checked this issue deeply and value was invisible because we haven't render any text, so @eyas2014 provided a correct fix ⭐️ But, we should render a placeholder instead of searchQuery by default as SUI does:

  <div class="ui floating dropdown labeled search button icon active visible">
    <input class="search" autocomplete="off" tabindex="0" value="VALUE" />
    <span class="text filtered">PLACEHOLDER</span>
  </div>

https://codesandbox.io/s/semantic-ui-react-example-55i18

@@ -1193,9 +1193,8 @@ export default class Dropdown extends Component {
search && searchQuery && 'filtered',
)
let _text = placeholder
if (searchQuery) {
_text = null
Copy link
Member

Choose a reason for hiding this comment

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

SUI renders text always and has styles to make it invisible 🐙

@layershifter layershifter merged commit 087593c into Semantic-Org:master Jul 7, 2019
@welcome
Copy link

welcome bot commented Jul 7, 2019

Congrats on merging your first pull request! 🎉🎉🎉

robot victory dance

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.

Dropdown: with button and search: search text isn't visible
4 participants