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

Add button autofocus prop to confirm modal #11756

Merged
merged 2 commits into from Sep 29, 2018
Merged

Add button autofocus prop to confirm modal #11756

merged 2 commits into from Sep 29, 2018

Conversation

ghost
Copy link

@ghost ghost commented Aug 15, 2018

@ant-design-bot
Copy link
Contributor

ant-design-bot commented Aug 15, 2018

Deploy preview for ant-design ready!

Built with commit 1d505f5

https://deploy-preview-11756--ant-design.netlify.com

@@ -9,6 +9,7 @@ import { getConfirmLocale } from './locale';
interface ConfirmDialogProps extends ModalFuncProps {
afterClose?: () => void;
close: (...args: any[]) => void;
autoFocusBtn: 'ok' | 'cancel';
Copy link
Member

Choose a reason for hiding this comment

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

Consider #11055 and we can set autoFocusBtn to null|false|undefined for no focused button.

Copy link
Member

Choose a reason for hiding this comment

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

autoFocusBtn => autoFocusButton

@@ -28,6 +29,7 @@ const ConfirmDialog = (props: ConfirmDialogProps) => {
const okText = props.okText ||
(okCancel ? runtimeLocale.okText : runtimeLocale.justOkText);
const cancelText = props.cancelText || runtimeLocale.cancelText;
const focus = props.autoFocusBtn || 'ok';
Copy link
Member

Choose a reason for hiding this comment

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

Don't rename autoFocusBtn;

@afc163
Copy link
Member

afc163 commented Aug 16, 2018

ci failed

@ghost
Copy link
Author

ghost commented Aug 17, 2018

@afc163 Done.
Don't know about ci failure, errors don't seem related to these changes at all.

@afc163
Copy link
Member

afc163 commented Aug 17, 2018

Don't forget update API documentation.

@@ -9,6 +9,7 @@ import { getConfirmLocale } from './locale';
interface ConfirmDialogProps extends ModalFuncProps {
afterClose?: () => void;
close: (...args: any[]) => void;
autoFocusButton?: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not:

autoFocusButton?: null | 'ok' | 'cancel'

?

@ghost
Copy link
Author

ghost commented Aug 17, 2018

@afc163 Can I add this feature to Modal component aswell?

@afc163
Copy link
Member

afc163 commented Aug 17, 2018

@erwin-k That is cool

@ghost
Copy link
Author

ghost commented Aug 17, 2018

@afc163 In the end didn't do it for Modal component cause it'd only focus on first open.

@@ -28,6 +29,7 @@ const ConfirmDialog = (props: ConfirmDialogProps) => {
const okText = props.okText ||
(okCancel ? runtimeLocale.okText : runtimeLocale.justOkText);
const cancelText = props.cancelText || runtimeLocale.cancelText;
const autoFocusButton = props.autoFocusButton === null ? false : props.autoFocusButton || 'ok'
Copy link
Contributor

@picodoth picodoth Aug 18, 2018

Choose a reason for hiding this comment

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

why do we need this line? We can simply use props.autoFocusButton directly.

const { autoFocusButton = 'ok' } = this.props;

Copy link
Author

Choose a reason for hiding this comment

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

Wouldn't that give 'ok' when null is passed? Passing null serves to disable autofocus in this case.

@@ -62,6 +62,7 @@ The properties of the object are follows:

| Property | Description | Type | Default |
| -------- | ----------- | ---- | ------- |
| autoFocusButton | Specify which button to autofocus | null\|string: `ok` `cancel` | `ok` |
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add something like this to the corresponding place in components/modal/index.zh-CN.md

| autoFocusButton | 指定自动获得焦点的按钮 | null\|string: `ok` `cancel` | `ok` |

@afc163
Copy link
Member

afc163 commented Aug 22, 2018

ci is still broken.

@ghost
Copy link
Author

ghost commented Aug 23, 2018

@afc163 I don't know about the failures, looks like it has nothing to do with these files. I have no time to investigate all that just for a pull request.

@afc163
Copy link
Member

afc163 commented Aug 24, 2018

Try rebase feature-3.9.0

@aaronplanell
Copy link
Contributor

Hello everyone,

Sorry, It's not related with this issue but I want to add a feature in the branch feature-3.9.0 and the button is not available. Instead of the button I have another one with the text "View #11756" and the link to this pull request. Is the branch blocked?

Thanks.

@afc163
Copy link
Member

afc163 commented Sep 3, 2018

Try rebase feature branch now.

@aaronplanell
Copy link
Contributor

Sorry @afc163 , how can I rebase the feature branch if I cannot download the code?

@afc163 afc163 changed the base branch from feature-3.9.0 to feature September 3, 2018 08:20
@afc163
Copy link
Member

afc163 commented Sep 3, 2018

feature-3.9.0 has been merged, you can checkout feature branch if you want to add new features.

@aaronplanell
Copy link
Contributor

aaronplanell commented Sep 3, 2018

Hello @afc163 ,

Maybe this is one of the problems with the PR of the thread:

  1. I fork the Ant Design project.
  2. I switch to the feature branch.
  3. I run nmp install
  4. I run npm t

There is a snapshot that fails:

Summary of all failing tests
 FAIL  components/tree/__tests__/demo.test.js
  ● renders ./components/tree/demo/basic.md correctly

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "renders ./components/tree/demo/basic.md correctly 1".

    - Snapshot
    + Received

Maybe this is the original problem and the reason because @erwin-k cannot merge 3 commits into ant-design:feature from erwin-k:feature-3.9.0.

It seems that before a previous change the Treenode Switcher was closed by default and now it is opened but the snapshots have not being updated.

@yesmeck yesmeck merged commit 2d8f9cc into ant-design:feature Sep 29, 2018
@codecov
Copy link

codecov bot commented Sep 29, 2018

Codecov Report

Merging #11756 into feature will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           feature   #11756      +/-   ##
===========================================
+ Coverage    92.31%   92.31%   +<.01%     
===========================================
  Files          211      211              
  Lines         5542     5543       +1     
  Branches      1594     1556      -38     
===========================================
+ Hits          5116     5117       +1     
- Misses         420      422       +2     
+ Partials         6        4       -2
Impacted Files Coverage Δ
components/modal/Modal.tsx 79.31% <ø> (ø) ⬆️
components/modal/confirm.tsx 93.02% <100%> (+0.16%) ⬆️
components/transfer/index.tsx 87.91% <0%> (ø) ⬆️
components/time-picker/index.tsx 81.96% <0%> (ø) ⬆️

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 fcb546e...1d505f5. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants