Skip to content

[Lens] Fix mobile view#96957

Merged
mbondyra merged 6 commits intoelastic:masterfrom
mbondyra:lens/fix_big_zoom
Apr 23, 2021
Merged

[Lens] Fix mobile view#96957
mbondyra merged 6 commits intoelastic:masterfrom
mbondyra:lens/fix_big_zoom

Conversation

@mbondyra
Copy link
Copy Markdown
Contributor

@mbondyra mbondyra commented Apr 13, 2021

Summary

Fixes #89077

Screenshot 2021-04-22 at 10 02 55

Open dimension panel:
Screenshot 2021-04-22 at 10 18 33

Discussion points (updated after @MichaelMarcialis comment)

  1. Heights of the three main sections - suggested solution: accordions for collapsing the sections - seems like a bigger task
  2. Order of the sections- keep as it is
  3. Open config panel for mobile should take the whole viewport

What this PR addresses:

  • Possibility of scrolling for mobile view
  • fixing 3 sections layout to display one after another instead of side by side:

Screenshot 2021-04-22 at 10 33 18

  • the query bar was unnecesarilly fixed as the rest of the header which was leading to very small scrollable space - I aligned it with how visualize solves it

Apr-22-2021 10-35-20

  • some overflowing elements were fixed, eg:

Screenshot 2021-04-22 at 10 39 37

  • some element are supposed to be displayed side by side were fixed too, eg:

Screenshot 2021-04-22 at 10 51 57

Issues I left untouched

I also spot some issues that I didn't address in this PR, as they feel more complicated or they require design input. Still, I think it is worth to merge this PR in this form after review and then iterate on better responsive design.

Screenshot 2021-04-22 at 11 02 09

@mbondyra mbondyra added Feature:Lens Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.13.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Apr 14, 2021
@mbondyra mbondyra force-pushed the lens/fix_big_zoom branch from 5c418a1 to 1ea7ff6 Compare April 19, 2021 10:00
@MichaelMarcialis
Copy link
Copy Markdown
Contributor

Hey, @mbondyra! Nice work on this a11y fix. Here's some of my initial thoughts based on your "doubts" list.

Heights of the three main sections

Regarding the index pattern and subsequent field list content, I believe we can potentially address the height concerns here by collapsing the section down (possibly like an accordion) at the breakpoint threshold where the Lens interface is laid out in a single column. By default, it could be collapsed (in high zoom or small viewport size situations) to avoid taking up an enormous amount of space, but still accessible when the user needs it. This also helps bring the visualization higher on the page.

Regarding the chart layers content, some side conversations have been happening elsewhere, but I think a few folks feel that it would be nice in general in general for chart layers to be able to collapse in some way as well. Doing so would potentially help with the height concerns you raise here (though I would think this to be secondary to the collapsing of the index pattern and field list content, described above).

Order of the sections (should the datasource panel or workspace be the first one?)

Assuming the above suggestion of collapsing the index pattern and field list content is one that resonates with you as well, my initial instinct would be to keep the order of sections as we currently have them: 1) index pattern and fields list, 2) workspace, 3) layers. I think this order continues to make sense at high zoom or small viewport size situations, as interaction with the fields list still remains the fastest way for a user to craft a visualization (i.e. adding to a workspace). Also, the workspace and layers are somewhat beholden to the fields list, as they both accept the dragging of fields to them, making this order of elements make all the more sense.

This is the view with open config panel. Is this how we want to solve it?

If possible, I think it would be nice to treat the configuration flyout as a fully overlaid modal experience for, just as the default EUI flyouts currently do (see GIF below). When opened, the configuration flyout overlays the entire experience and focuses the user's attention only on the contents of the flyout. After the user makes their changes, they may close the flyout in order to once again see the content it was covering. Would that be possible?

flyout-gif

That said, let me know if we want to begin a formal design effort for a fully responsive version of the Lens UI and I can work with the team on how to prioritize.

@spalger spalger added v7.14.0 and removed v7.13.0 labels Apr 21, 2021
@mbondyra mbondyra force-pushed the lens/fix_big_zoom branch from 1ea7ff6 to 7511a51 Compare April 22, 2021 07:56
@mbondyra
Copy link
Copy Markdown
Contributor Author

Hi @MichaelMarcialis I've addressed your comments, please see the updated description of this PR.

That said, let me know if we want to begin a formal design effort for a fully responsive version of the Lens UI and I can work with the team on how to prioritize.

Yeah, accordions you mention seem like bigger effort also from developer's side – let's talk about this during weekly.

That being said, I still consider this PR an improvement and fixing the accessibility issue that I linked. Could we still merge it in this form and then improve on top, when we start working on the fully responsive version of Lens? (if we decide to do so)

@mbondyra mbondyra marked this pull request as ready for review April 22, 2021 08:48
@mbondyra mbondyra requested a review from a team April 22, 2021 08:48
@mbondyra mbondyra requested review from a team as code owners April 22, 2021 08:48
@elasticmachine
Copy link
Copy Markdown
Contributor

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

@mbondyra mbondyra removed the request for review from a team April 22, 2021 08:50
@mbondyra mbondyra force-pushed the lens/fix_big_zoom branch from a2f5f52 to 1b47c01 Compare April 22, 2021 09:05
@MichaelMarcialis
Copy link
Copy Markdown
Contributor

Could we still merge it in this form and then improve on top, when we start working on the fully responsive version of Lens? (if we decide to do so)

I think that's a good approach. What you have here still provides a lot of great fixes. Will review shortly!

@myasonik
Copy link
Copy Markdown
Contributor

This PR will bring Lens back to 0 open a11y issues and I'm so excited 🎉

Copy link
Copy Markdown
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together, Marta. Works as advertised and serves as a great first step in making Lens more responsive.

@elastic elastic deleted a comment from kibanamachine Apr 22, 2021
@mbondyra mbondyra changed the title [Lens] fix mobile zoom view [Lens] Fix mobile view Apr 22, 2021
@mbondyra mbondyra added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 22, 2021
Copy link
Copy Markdown
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Overall LGTM, tested in Firefox. Found one issue with scrollbars.

}
@include euiBreakpoint('xs', 's', 'm') {
@include euiFlyout;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I ran into a similar issue here with the flyout being part of the right side container, but we should really disable scrolling while the flyout is open. I think this is something the EUI components can do for us?

Screen Shot 2021-04-22 at 4 36 32 PM

@mbondyra
Copy link
Copy Markdown
Contributor Author

mbondyra commented Apr 23, 2021

@wylieconlon so the problem is that we don't use the eui EuiFlyout for our usecase (we mount it inside the config panel and that's a bit problematic). We use pieces of EuiFlyout - the styling, and internals, like EuiFlyoutHeader and the rest. I was trying to adapt the EuiFlyout and use it, but with no success and I suspect there's not really a good option to do it. Caroline wrote the original code, so I trust this solution and the right usage of eui.

There's no util that would make a body non-scrollable coming from eui so I just decided to do simple overflow: hidden on body when flyout is open. If there's a better way, I'm happy to hear it!

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 948.5KB 964.6KB +16.0KB

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

@mbondyra mbondyra merged commit 2f66397 into elastic:master Apr 23, 2021
@mbondyra mbondyra deleted the lens/fix_big_zoom branch April 23, 2021 13:56
mbondyra added a commit to mbondyra/kibana that referenced this pull request Apr 23, 2021
mbondyra added a commit to mbondyra/kibana that referenced this pull request Apr 23, 2021
mbondyra added a commit that referenced this pull request Apr 23, 2021
mbondyra added a commit that referenced this pull request Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Lens release_note:fix Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.13.0 v7.14.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(Accessibility) - Lens page does not support reflow per WCAG 2.1 - 1.4.10

7 participants