Skip to content

feat: implement capitalization type option for prompt dialogs #6325

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.

Sign up for GitHub

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

Jump to bottom
Merged
vakrilov merged 4 commits into from
Oct 5, 2018
Merged

feat: implement capitalization type option for prompt dialogs #6325

vakrilov merged 4 commits into from
Oct 5, 2018

Conversation

@hettiger
Copy link
Contributor

@hettiger hettiger commented Sep 30, 2018 •
edited
Loading

Allow setting auto capitalization to "none", "all", "sentences" and "words".
This feature works on both Android and iOS.
Closes: #6326

PR Checklist

What is the current behavior?

Prompt dialogs do not support auto capitalization.
This leads to a bad user experience and bad input.

What is the new behavior?

It's now possible to set a capitalization type on the PromptOptions object.

Example:

const options: PromptOptions = {
    title: "Title",
    message: "Message",
    inputType: inputType.text,
    okButtonText: "OK",
    cancelButtonText: "Cancel",
    cancelable: true,
    capitalizationType: capitalizationType.sentences
};

(The new property is: capitalizationType: capitalizationType.sentences)

There are no breaking changes to be expected.
The new property is optional.

Test Results

iOS

=== ALL TESTS COMPLETE ===
1612 OK, 6 failed
DURATION: 28714.39 ms
=== END OF TESTS ===
CONSOLE ERROR file:///app/tns_modules/tns-core-modules/trace/trace.js:166:30: Test: FLEXBOXLAYOUT.testJustifyContent_flexEnd_withParentPadding FAILED:  Actual: <654>(number). Expected: <686>(number)
CONSOLE ERROR file:///app/tns_modules/tns-core-modules/trace/trace.js:166:30: Test: SAFEAREALAYOUT.test_grid_component_cells_layout_in_safe_area_action_bar_hidden FAILED: cell height should be equal - cell11<432> - cell21<431> Actual: <432>(number). Expected: <431>(number)
CONSOLE ERROR file:///app/tns_modules/tns-core-modules/trace/trace.js:166:30: Test: SAFEAREALAYOUT.test_grid_component_cells_layout_in_safe_area_tab_bar FAILED: cell height should be equal - cell11<398> - cell21<399> Actual: <398>(number). Expected: <399>(number)
CONSOLE ERROR file:///app/tns_modules/tns-core-modules/trace/trace.js:166:30: Test: SAFEAREALAYOUT.test_grid_nested_grid_cells_layout_beyond_safe_area_action_bar_hidden FAILED: cell11 height<432> not less or equal cell21 height<431>
CONSOLE ERROR file:///app/tns_modules/tns-core-modules/trace/trace.js:166:30: Test: SAFEAREA-LISTVIEW.test_grid_nested_list_views_layout_in_safe_area_action_bar_hidden FAILED: cell11 height<432> not less or equal cell21 height<431>
CONSOLE ERROR file:///app/tns_modules/tns-core-modules/trace/trace.js:166:30: Test: SAFEAREA-SCROLL-VIEW.test_grid_component_cells_layout_in_safe_area_action_bar_hidden FAILED: cell11 height<432> not less or equal cell21 height<431>
CONSOLE INFO file:///app/tns_modules/tns-core-modules/trace/trace.js:160:29: Test: Tests EOF!

Android

JS: === ALL TESTS COMPLETE ===
JS: 1489 OK, 2 failed
JS: DURATION: 47099.06 ms
JS: === END OF TESTS ===
JS: Test: FLEXBOXLAYOUT.testAlignContent_spaceAround FAILED: Label<text3>.bottom:1368 is not 52.5 of FlexboxLayout<flexbox>.bottom:1422
JS: Test: FLEXBOXLAYOUT.testAlignContent_spaceAround_flexDirection_column FAILED: Label<text3>.right:906 is not 52.5 of FlexboxLayout<flexbox>.right:960
JS: Test: Tests EOF!

@hettiger
feat: implement capitalization type option for prompt dialogs
7ed49ae 
Allow setting auto capitalization to "none", "all", "sentences" and "words".
This feature works on both Android and iOS.
@ghost ghost added the ♥ community PR label Sep 30, 2018
@hettiger hettiger mentioned this pull request Sep 30, 2018
Closed
@hettiger
Copy link
Contributor Author

hettiger commented Oct 1, 2018

I'm not familiar with NativeScript e2e testing. Am I expected to provide tests or does the label »e2e test needed« mean that someone of the NativeScript team will take care of it?

I'd be happy to provide the tests myself but I'll probably need some help / reviews and input to get me started on NativeScript e2e testing.

Am I on the right track? Is there anything special about this repository that differs from the guide?
Are there any additional docs to get me started that I've missed?

Please let me know how we can continue with this PR @vakrilov

@vakrilov
Copy link
Contributor

vakrilov commented Oct 1, 2018

Hey @hettiger
First of all - thanks for the awesome PR!
The few failing test are not caused by your code, so don't worry about them.

You are not expected to provide and e2e test about this PR (the e2e test needed label is for internal use for now). It would be great if you could extend the existing test page for dialogs with buttons showing a dialogs with the new capitalization options. Our QAs will just add the e2e tests using this page once the PR is merged. Once that is ready - I think this one is ready to land in master.

@hettiger
Copy link
Contributor Author

hettiger commented Oct 3, 2018

Thank you for your feedback @vakrilov

  • I've added the requested buttons to the existing test page.
  • The test results remain the same after the new commit.

Let me know if there's anything missing.

@vakrilov
Copy link
Contributor

vakrilov commented Oct 3, 2018

LGTM!
We are waiting merge some PRs that should fix the broken tests.
We will update you branch after that and hopefully everything should be green.
Nothing to do from your side for now.

@vakrilov
Copy link
Contributor

vakrilov commented Oct 5, 2018

test

@ghost ghost assigned vakrilov Oct 5, 2018
@ghost ghost added in progress and removed ♥ community PR labels Oct 5, 2018
@vakrilov
Copy link
Contributor

vakrilov commented Oct 5, 2018

test

@vakrilov vakrilov merged commit ae6a661 into NativeScript:master Oct 5, 2018
@ghost ghost removed the in progress label Oct 5, 2018
@MartoYankov MartoYankov added the docs needed Additional documentation on this issue/PR is needed label Oct 6, 2018
@radeva
Copy link

radeva commented Nov 19, 2018

Hi @hettiger ,

Congratulations on being one of the winners in the {N} First-time contributors contest!

You can claim your prize by contacting us at nativescriptwinners[at]progress.com not later than Nov 30th 2018 . Make sure you send us the following info: 
👉Your full name 
👉Your email 
👉Your country or residence 

Best regards,
The NativeScript Team

@NickIliev NickIliev removed the docs needed Additional documentation on this issue/PR is needed label Jan 11, 2019
This was referenced Jan 11, 2019
Merged
Merged
@lock
Copy link

lock bot commented Mar 10, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@vakrilov vakrilov vakrilov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vakrilov vakrilov

Labels

cla: yes ♥ community PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Prompt dialog: An option for auto capitalization is missing

7 participants

@hettiger @vakrilov @radeva @SvetoslavTsenov @MartoYankov @ns-bot @NickIliev