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(searchbar): add inputmode property #18980

Merged
merged 2 commits into from Aug 8, 2019

Conversation

@5im0n
Copy link
Contributor

commented Aug 2, 2019

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Ionic searchbar component can now have a inputmode property.
The default value is set to search. https://developer.mozilla.org/fr/docs/Web/HTML/Attributs_universels/inputmode

Does this introduce a breaking change?

  • Yes
  • No

Other information

@brandyscarney brandyscarney requested a review from manucorporat Aug 2, 2019

* A hint to the browser for which keyboard to display.
* This attribute applies when the value of the type attribute is `"text"`, `"password"`, `"email"`, or `"url"`. Possible values are: `"verbatim"`, `"latin"`, `"latin-name"`, `"latin-prose"`, `"full-width-latin"`, `"kana"`, `"katakana"`, `"numeric"`, `"tel"`, `"email"`, `"url"`.
*/
@Prop() inputmode = 'search';

This comment has been minimized.

Copy link
@manucorporat

manucorporat Aug 2, 2019

Member

If there is a well-known set of values why not define it in the type itself:

Suggested change
@Prop() inputmode = 'search';
@Prop() inputmode: 'verbatim' | 'latin' | 'latin-name' | 'latin-prose' | 'full-width-latin' | 'kana' | 'katakana', 'numeric' | 'tel' | 'email' | 'url' = 'search';

This comment has been minimized.

Copy link
@5im0n

5im0n Aug 3, 2019

Author Contributor

It make sence, but I think we should review the set of values.
As we can read in the HTML spec somes attributes like latin, latin-prose... doens't exist.

As the spec say, i suggest:

@Prop() inputmode: 'none' | 'text' | 'tel' | 'url' | 'email' | 'numeric' | 'decimal' | 'search' = 'search';

We should update the inputmode for the ion-input component as well !

This comment has been minimized.

Copy link
@manucorporat

manucorporat Aug 5, 2019

Member

agreed, it should be based in what the spec says, and updating ion-inputs would be great but i would update ion-input in a different PR though, it might be a breaking change

This comment has been minimized.

Copy link
@5im0n

5im0n Aug 6, 2019

Author Contributor

I push a new commit to set the default values that match the specs.

Do you want that a create a new PR to do the same thing on the ion-input ?

@manucorporat manucorporat merged commit 1187dc2 into ionic-team:master Aug 8, 2019

1 check failed

build Workflow: build
Details

@5im0n 5im0n deleted the 5im0n:feat-searchbar-inputmode branch Aug 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.