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 bug with scrolling page after emoji group click #2587

Merged
merged 13 commits into from
Nov 8, 2018
Merged

Fix bug with scrolling page after emoji group click #2587

merged 13 commits into from
Nov 8, 2018

Conversation

msamsel
Copy link
Contributor

@msamsel msamsel commented Nov 7, 2018

What is the purpose of this pull request?

Bug fix

This PR contains

  • Unit tests
  • Manual tests

What changes did you make?

  • fix bug, when page was scrolled when group of emoji dropdown was clicked

Close #2571

@mlewand mlewand self-requested a review November 8, 2018 08:34
@mlewand mlewand self-assigned this Nov 8, 2018
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Navigating using hash links to h2 id'ed by a given string caused also outer page to scroll - this issue has been fixed correctly.

There are only couple of minor code styles thing and we're good to go.

@@ -115,7 +115,7 @@

assert.areSame( 0, emojiBlock.$.scrollTop, 'Emoji elements should be scrolled to the top.' );

doc.findOne( 'a[href="#flags"]' ).$.click();
doc.findOne( 'a[title="Flags"]' ).$.click();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're using title (that is localized) you have to force editor language to English. Otherwise it might get set to user-agent language, and test will fail because of translations.


1. Open emoji dropdown.
2. Click into emoji group.
### Expected:
Copy link
Contributor

Choose a reason for hiding this comment

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

Expected / unexpected headers are not followed with a colon.

doc.findOne( 'a[title="Flags"]' ).$.click();
assert.areEqual( scrollPosition, win.getScrollPosition().y, 'Window should not be scrolled down after clicking into navigation' );
}
finally {
Copy link
Contributor

Choose a reason for hiding this comment

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

finally should be in the same line with a previous curly brace.

parentHeight;

// There is need to resize iframe with test run from dashboard. To prevent situation,
// where foccusing element will additionaly scroll page
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// where foccusing element will additionaly scroll page
// where focusing element will additionally scroll page

typo

@Comandeer Comandeer self-requested a review November 8, 2018 09:22
@Comandeer Comandeer self-assigned this Nov 8, 2018
Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

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

I have one thought: couldn't it be done by adding preventDefault to link's click event, not by stripping [href] completely? Stripping [href] made links unfocusable elements, which results in e.g. lack of focus styles for groups.

Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

I added missing space and adjusted the changelog entry.

There were 2 minor issues that we don't have to fix right now, extracted to #2592.

@mlewand
Copy link
Contributor

mlewand commented Nov 8, 2018

Rebased onto latest master.

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.

3 participants