Skip to content

Conversation

@crisbeto
Copy link
Member

Based on top of #19834, changes the places where we were handling home/end to go through the key manager.

Also allows withHomeAndEnd to be turned off, similarly to what we're doing with the other methods.

cc @vanessanschmitt

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: minor This PR is targeted for the next minor release labels Jul 13, 2020
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 13, 2020
@crisbeto crisbeto force-pushed the 19834/home-end-refactor branch from 8d20b5d to 4d37216 Compare July 13, 2020 04:24
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM. nice cleanup


const event = createKeyboardEvent('keydown', END);
Object.defineProperty(event, 'shiftKey', { get: () => true });
Object.defineProperty(event, 'altKey', { get: () => true });
Copy link
Member

Choose a reason for hiding this comment

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

Just for context: We had to change this test because previously we didn't allow HOME/END with any modifier keys pressed. Now though we will start supporting those keys with the SHIFT modifier key, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly. I did it since, from what I can tell, home and end don't do much with a modifier anyway.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Jul 13, 2020
@wagnermaciel
Copy link
Contributor

@crisbeto Seems like this PR has some merge conflicts

@crisbeto
Copy link
Member Author

Rebased.

@crisbeto crisbeto force-pushed the 19834/home-end-refactor branch from 4d37216 to 5f8ccca Compare July 22, 2020 06:35
@mmalerba mmalerba removed the lgtm label Jul 31, 2020
@crisbeto crisbeto force-pushed the 19834/home-end-refactor branch from 5f8ccca to a9d52ca Compare August 2, 2020 13:09
Based on top of angular#19834, changes the places where we were handling home/end to go through the key manager.

Also allows `withHomeAndEnd` to be turned off, similarly to what we're doing with the other methods.
@crisbeto crisbeto force-pushed the 19834/home-end-refactor branch from a9d52ca to 23432bd Compare August 19, 2020 19:16
@wagnermaciel wagnermaciel merged commit d96de48 into angular:master Aug 21, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants