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(module:select): do not bring up keyboard when EnableSearch is false #1992

Conversation

anranruye
Copy link
Member

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • Component style update
  • Bundle size optimization
  • Performance optimization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

🔗 Related issue link

Fixes #1979

💡 Background and solution

📝 Changelog

Language Changelog
🇺🇸 English fix: non-searchable Select brings up keyboard on mobile devices
🇨🇳 Chinese

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • Changelog is provided or not needed

@github-actions
Copy link

github-actions bot commented Oct 1, 2021

@codecov
Copy link

codecov bot commented Oct 1, 2021

Codecov Report

Merging #1992 (1bf05bc) into master (1f2a7f6) will increase coverage by 1.86%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1992      +/-   ##
==========================================
+ Coverage   25.39%   27.25%   +1.86%     
==========================================
  Files         488      511      +23     
  Lines       32019    24405    -7614     
  Branches        0      233     +233     
==========================================
- Hits         8130     6652    -1478     
+ Misses      23889    17717    -6172     
- Partials        0       36      +36     
Impacted Files Coverage Δ
components/select/internal/SelectContent.razor 66.03% <ø> (+10.64%) ⬆️
components/select/Select.razor.cs 48.58% <50.00%> (+2.57%) ⬆️
components/select/SelectBase.cs 63.57% <100.00%> (+7.19%) ⬆️
components/select/internal/SelectContent.razor.cs 57.97% <100.00%> (+5.38%) ⬆️
components/core/Helpers/MemberPath/PathNode.cs 46.15% <0.00%> (-8.85%) ⬇️
components/core/Reflection/TypeDefined.cs 77.77% <0.00%> (-7.94%) ⬇️
components/input/InputGroup.razor.cs 87.50% <0.00%> (-6.95%) ⬇️
components/tabs/TabPane.razor 86.66% <0.00%> (-6.20%) ⬇️
...ponents/date-picker/types/DatePickerPlaceholder.cs 22.72% <0.00%> (-4.20%) ⬇️
components/input/TextArea.razor.cs 77.52% <0.00%> (-3.05%) ⬇️
... and 425 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 1f2a7f6...1bf05bc. Read the comment docs.

Copy link
Member

@anddrzejb anddrzejb left a comment

Choose a reason for hiding this comment

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

I found 1 problem related to this issue & 1 problem not directly related, but maybe you could fix it here as well:
image

First problem is self-explanatory I believe. Second problem - whole idea of tags mode is to allow input (so users can enter new values that will be added). My feeling is that when in tagsmode, the EnableSelect parameter should be set to true when not set by users.

@anranruye
Copy link
Member Author

@anddrzejb the one in the image is a "multiple" mode Select, not "tag" mode.

@anddrzejb
Copy link
Member

First is default, second is multiple and third is tags.

@anranruye
Copy link
Member Author

anranruye commented Oct 2, 2021

So can the third one open the keyboard?

And EnableSearch is a parameter property. I don't think we should modify it manually. Microsoft suggests that we should never change a parameter property manually and all parameter properties should be auto-implemented. Though sometimes we don't follow this design pattern, we should follow it as possible.

@anddrzejb
Copy link
Member

My belief is that the third one, that is tags, should open the keyboard. The basic functionality of the Mode="tags" requires input to be active, which is not active now - although mouse pointer suggests it is:
image
Without input active, the Select works like multiple (for the most part). Regarding changing the parameter - you are right, but we could convert to full property and use an underlying boolean field. From the setter we could detect if the value was changed by the user or if it is a default given by the Select component and act appropriately.
This change I am discussing above is not directly connected with this PR. Of course, the fact that keyboard shows on the multiple Select when EnableSearch = false is a part of this PR and should be addressed. The tags situation is just a proposal, that maybe you could fix it here. If you'd like to leave it for later, I will not oppose.

Copy link
Member

@anddrzejb anddrzejb left a comment

Choose a reason for hiding this comment

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

Great work, thaks!

@ElderJames ElderJames merged commit 6f51e58 into ant-design-blazor:master Oct 8, 2021
ElderJames pushed a commit that referenced this pull request Oct 16, 2021
…lse (#1992)

* fix(module:select): do not bring up keyboard when EnableSearch is false

* Update SelectContent.razor

* consider tags mode as searchEnabled
@anranruye anranruye deleted the FixSelectBringMobileDeviceKeyboard branch October 29, 2021 13:20
ElderJames pushed a commit that referenced this pull request Apr 23, 2022
…lse (#1992)

* fix(module:select): do not bring up keyboard when EnableSearch is false

* Update SelectContent.razor

* consider tags mode as searchEnabled
ElderJames pushed a commit that referenced this pull request Apr 30, 2022
…lse (#1992)

* fix(module:select): do not bring up keyboard when EnableSearch is false

* Update SelectContent.razor

* consider tags mode as searchEnabled
ElderJames pushed a commit that referenced this pull request Sep 6, 2022
…lse (#1992)

* fix(module:select): do not bring up keyboard when EnableSearch is false

* Update SelectContent.razor

* consider tags mode as searchEnabled
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.

Non-searchable Select brings up keyboard on mobile devices
3 participants