[Discover] Deangularize Skip to bottom button #69811

dej611 · 2020-06-24T15:25:43Z

Summary

This PR contains the following features/fixes:

✨ "deangularized" the Skip to Bottom button in Discover
💄 Fix a styling conflict between the new EUI button and the dscSkipButton class
🐛 Fix Safari issue with the button and behaviour (not clickable + wrong size + not scrolling)
✅ Add basic testing

Note: To test on Safari make sure to enable Tab behaviour in Preferences > Advanced > Press Tab to highlight...

Updated screenshots

Desktop:

Smartphone:

Tablet:

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

elasticmachine · 2020-06-25T13:19:01Z

Pinging @elastic/kibana-app (Team:KibanaApp)

kertal

Code LGTM, this is a great debut in the discover world 🥇 . nice 👁️ for detail. About the styling for small screen, don't think we need adaption here, button won't be necessary or will be solved differently once data grid will be the center of Discover, which will take some time. #67259.

cchaos

Hey @dej611! Quick design pass for you.

cchaos · 2020-06-25T14:45:25Z

src/plugins/discover/public/application/components/skip_bottom_button/skip_bottom_button.tsx

+      <EuiButton onClick={onClick} iconType="arrowDown" className="dscSkipButton">
+        <FormattedMessage id="discover.skipToBottomButtonLabel" defaultMessage="Skip to bottom" />
+      </EuiButton>


EUI actually has an EuiSkipLink precisely for this use case that would be useful here without needing the extra CSS to push it off screen as EuiSkipLink will automatically hide/show when tabbing to it. I would also make the text more descriptive than "bottom".

Suggested change

<EuiButton onClick={onClick} iconType="arrowDown" className="dscSkipButton">

<FormattedMessage id="discover.skipToBottomButtonLabel" defaultMessage="Skip to bottom" />

</EuiButton>

<EuiSkipLink size="s" onClick={onClick} className="dscSkipButton" destinationId="">

<FormattedMessage id="discover.skipToBottomButtonLabel" defaultMessage="Skip to table" />

</EuiSkipLink>

Sounds good. I looked in the button section without searching for Skip effectively in the doc. Good suggestion, I'm going to use it.

nice, didn't know this component 👍

cchaos · 2020-06-25T14:48:22Z

src/plugins/discover/public/application/_discover.scss

  left: -10000px;
  top: $euiSizeXS;


Then this would simply become.

Suggested change

left: -10000px;

top: $euiSizeXS;

right: $euiSizeXS;

top: $euiSizeXS;

And you can delete everything below this.

dej611 · 2020-06-26T08:28:21Z

Pushed suggestion fixes. The Safari fix in this PR has been removed and rather fixed up the line on the EUI EuiSkipLink itself, available in the next version of the package.

cchaos

A couple last suggestions

cchaos · 2020-06-26T14:04:56Z

src/plugins/discover/public/application/_discover.scss

@@ -100,16 +100,6 @@ discover-app {

 .dscSkipButton {
  position: absolute;
-  left: -10000px;
+  right: $euiSizeXS;


Suggested change

right: $euiSizeXS;

right: $euiSizeM;

cchaos · 2020-06-26T14:09:36Z

src/plugins/discover/public/application/angular/discover.html

@@ -141,7 +130,7 @@ <h1 class="euiScreenReaderOnly">{{screenTitle}}</h1>
                on-remove-column="removeColumn"
              ></doc-table>

-              <a tabindex="0" id="discoverBottomMarker"></a>
+              <a tabindex="0" id="discoverBottomMarker">&#8203;</a>


This symbol actually adds a line of whitespace, is this intended?

Yes. From my tests Safari won't scroll to the marker if empty. For this reason I had to add this empty space character to the node.

cchaos · 2020-06-26T14:11:00Z

src/plugins/discover/public/application/components/skip_bottom_button/skip_bottom_button.tsx

+    <I18nProvider>
+      <EuiSkipLink
+        size="s"
+        iconType="arrowDown"


We usually reserve the arrow icons to indicate popovers or disclosure style content not directional. I think it can be omitted entirely.

Suggested change

iconType="arrowDown"

Ah ok. I've set it to match the previous button, but if that is not longer required I'll remove it.

dej611 · 2020-06-29T08:10:22Z

@elasticmachine merge upstream

dej611 · 2020-06-29T12:22:33Z

@elasticmachine merge upstream

kertal · 2020-06-29T13:42:03Z

checked again, still LGTM 👍

kibanamachine · 2020-06-29T13:59:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 7d8f13f

Build metrics

✅ unchanged

History

💚 Build #56755 succeeded 5a35009
💚 Build #56653 succeeded 2db9cbf6cd704c00bac0127a24c7619e7b9dc1eb
💔 Build #56587 failed a17673deef6237a3f3b9bf37b255218577ee8031
💚 Build #56210 succeeded ca2be4e20a163b60c6a63d967f7410ba6beb8ff1
💔 Build #55949 failed 106b5d898057c8d5bc418bb9fc271a89eb60f13d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cchaos

LGTM! Safari and TS fixes for EuiSkipLink will get updated with the latest EUI

Co-authored-by: Elastic Machine <[email protected]>

…ata-streams * 'master' of github.com:elastic/kibana: (50 commits) [Logs UI] [Alerting] "Group by" functionality (elastic#68250) [Discover] Deangularize Skip to bottom button (elastic#69811) Implement recursive plugin discovery (elastic#68811) Use ts-expect-error in platform code (elastic#69883) [SIEM][Detection Engine][Lists] Moves getQueryFilter to common folder for use by both front and backend [Ingest Manager][SECURITY SOLUTION] adjust config reassign link and add roundtrip to Reassignment flow (elastic#70208) [Security][Lists] Add API functions and react hooks for value list APIs (elastic#69603) [ILM] Fix bug when clearing priority field (elastic#70154) [Platform][Security] Updates cluster_manager ignorePaths to include security scripts (elastic#70139) [IngestManager] Allow to filter agent by packages (elastic#69731) [code coverage] exclude folders: test_helpers, tests_bundle (elastic#70199) [Metrics UI] UX improvements for saved views (elastic#69910) [APM] docs: unique transaction troubleshooting (elastic#69831) Cross cluster search functional test with minimun privileges assigned to the test_user (elastic#70007) [Maps] choropleth layer wizard (elastic#69699) Make custom errors by extending Error (elastic#69966) [Ingest Manager] Support updated package output structure (elastic#69864) Resolver test coverage (elastic#70246) Async Discover search test (elastic#64388) [ui-shared-deps] include styled-components (elastic#69322) ... # Conflicts: # x-pack/plugins/snapshot_restore/server/types.ts

…bana into alerting/consumer-based-rbac * 'alerting/consumer-based-rbac' of github.com:gmmorris/kibana: (49 commits) [Discover] Deangularize Skip to bottom button (elastic#69811) Implement recursive plugin discovery (elastic#68811) Use ts-expect-error in platform code (elastic#69883) [SIEM][Detection Engine][Lists] Moves getQueryFilter to common folder for use by both front and backend [Ingest Manager][SECURITY SOLUTION] adjust config reassign link and add roundtrip to Reassignment flow (elastic#70208) [Security][Lists] Add API functions and react hooks for value list APIs (elastic#69603) [ILM] Fix bug when clearing priority field (elastic#70154) [Platform][Security] Updates cluster_manager ignorePaths to include security scripts (elastic#70139) [IngestManager] Allow to filter agent by packages (elastic#69731) [code coverage] exclude folders: test_helpers, tests_bundle (elastic#70199) [Metrics UI] UX improvements for saved views (elastic#69910) [APM] docs: unique transaction troubleshooting (elastic#69831) Cross cluster search functional test with minimun privileges assigned to the test_user (elastic#70007) [Maps] choropleth layer wizard (elastic#69699) Make custom errors by extending Error (elastic#69966) [Ingest Manager] Support updated package output structure (elastic#69864) Resolver test coverage (elastic#70246) Async Discover search test (elastic#64388) [ui-shared-deps] include styled-components (elastic#69322) SECURITY-ENDPOINT: add host properties (elastic#70238) ...

Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>

dej611 requested a review from kertal June 24, 2020 15:25

dej611 self-assigned this Jun 24, 2020

dej611 added bug Fixes for quality problems that affect the customer experience Feature:Discover Discover Application Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0 labels Jun 24, 2020

dej611 linked an issue Jun 24, 2020 that may be closed by this pull request

[Discover] Deangularize Skip to bottom button #65453

Closed

dej611 added release_note:skip Skip the PR/issue when compiling release notes and removed bug Fixes for quality problems that affect the customer experience labels Jun 25, 2020

dej611 marked this pull request as ready for review June 25, 2020 13:18

dej611 requested a review from a team June 25, 2020 13:18

dej611 requested a review from a team as a code owner June 25, 2020 13:18

kertal approved these changes Jun 25, 2020

View reviewed changes

cchaos reviewed Jun 25, 2020

View reviewed changes

cchaos reviewed Jun 26, 2020

View reviewed changes

dej611 added 8 commits June 26, 2020 18:18

🚚 Deangularized skip to bottom button

ef148c9

🐛 Improve button look + Safari click fix

35c243b

:white_mark_check: Add basic tests

cc4530d

📸 Update snapshot test

16789c7

🔥 Remove unstable snapshot test

5dbcf9f

👌 Integrated suggestions for EUI components

74f8eb9

🏷️ Make TS ignore the onClick prop for now

effd662

👌 Integrate suggestions

5a35009

dej611 force-pushed the deangularization/discover-bottom-button branch from 2db9cbf to 5a35009 Compare June 26, 2020 16:24

Merge branch 'master' into deangularization/discover-bottom-button

603dc3a

Merge branch 'master' into deangularization/discover-bottom-button

7d8f13f

kertal mentioned this pull request Jun 29, 2020

[Discover] Deangularize context_app.html #70211

Closed

dej611 requested a review from cchaos June 29, 2020 18:20

cchaos approved these changes Jun 29, 2020

View reviewed changes

dej611 merged commit a40e58e into elastic:master Jun 30, 2020

dej611 deleted the deangularization/discover-bottom-button branch June 30, 2020 08:59

dej611 mentioned this pull request Jun 30, 2020

[7.x] [Discover] Deangularize Skip to bottom button (#69811) #70296

Merged

dej611 added a commit to dej611/kibana that referenced this pull request Jun 30, 2020

[Discover] Deangularize Skip to bottom button (elastic#69811)

652812d

Co-authored-by: Elastic Machine <[email protected]>

dej611 added a commit that referenced this pull request Jun 30, 2020

[7.x] [Discover] Deangularize Skip to bottom button (#69811) (#70296)

5be7c00

Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

Bamieh pushed a commit to Bamieh/kibana that referenced this pull request Jul 1, 2020

[Discover] Deangularize Skip to bottom button (elastic#69811)

d9e06b1

Co-authored-by: Elastic Machine <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Deangularize Skip to bottom button #69811

[Discover] Deangularize Skip to bottom button #69811

dej611 commented Jun 24, 2020 •

edited

Loading

elasticmachine commented Jun 25, 2020

kertal left a comment

cchaos left a comment

cchaos Jun 25, 2020

dej611 Jun 25, 2020

kertal Jun 25, 2020

cchaos Jun 25, 2020

dej611 commented Jun 26, 2020

cchaos left a comment

cchaos Jun 26, 2020

cchaos Jun 26, 2020

dej611 Jun 26, 2020

cchaos Jun 26, 2020

dej611 Jun 26, 2020

dej611 commented Jun 29, 2020

dej611 commented Jun 29, 2020

kertal commented Jun 29, 2020

kibanamachine commented Jun 29, 2020

cchaos left a comment

[Discover] Deangularize Skip to bottom button #69811

[Discover] Deangularize Skip to bottom button #69811

Conversation

dej611 commented Jun 24, 2020 • edited Loading

Summary

Checklist

elasticmachine commented Jun 25, 2020

kertal left a comment

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dej611 commented Jun 26, 2020

cchaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dej611 commented Jun 29, 2020

dej611 commented Jun 29, 2020

kertal commented Jun 29, 2020

kibanamachine commented Jun 29, 2020

💚 Build Succeeded

Build metrics

History

cchaos left a comment

Choose a reason for hiding this comment

dej611 commented Jun 24, 2020 •

edited

Loading