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

Streamline UI and allow embedding in iframes #3885

Merged
merged 38 commits into from
Mar 20, 2019
Merged

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Mar 12, 2019

tldr:

  • allows to embed wk within an iframe
    • logging in does not work due to third-party cookies, but there will be a warning stating this
    • the golden layout config will not be persisted within an iframe and the default layout is switched to a two-column layout
  • the wk-core "toolbar" was merged into the navbar to save vertical space and make the overall UI leaner
  • some stylistic tweaks here and there

URL of deployed dev instance (used for testing):

Steps to test:

  • mainly play around with screen sizes and check basic functionality

Issues:

Screenshots

image
image
For small devices:
image


@philippotto
Copy link
Member Author

@daniel-wer Maybe you can have a look at https://www.playframework.com/documentation/ja/2.3.x/SecurityHeaders to see whether we should keep some of these security measures?

@philippotto philippotto self-assigned this Mar 12, 2019
@philippotto philippotto changed the title [WIP] Streamline UI and allow embedding in iframes Streamline UI and allow embedding in iframes Mar 18, 2019
@philippotto philippotto requested a review from daniel-wer March 18, 2019 15:31
Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Incredible PR! I love how much thought you put into every single part of this, very well executed! 🏆
I can assure, opening up the "old" version of wK in a few weeks will be very strange (it already is for me, and I only reviewed this PR ^^), this is just so much better 👍

Things I noticed during testing:

  • The spinner and percentage when saving large update actions no longer have a margin and are too close together.

  • The Dasboard Icon does not show a title/tooltip when hovering.

  • The highlighting of the visited sub page for the admin pages doesn't seem to work, "Dashboard" is always highlighted for me, regardless of whether I'm looking at statistics or task types pages, etc...

  • The version restore view is a little too large at the top / the navbar is too tall when being logged out

  • When logged out and looking at a tmpscratch dataset, the tmpscratch and anonymous user avatar are too close together
    tmpscratch-user

  • What would be a good way to test the iframe-embeddability? Should we include a test .html page in the repo, which can simply be opened?

  • I don't think this has been introduced in this PR, but some of the buttons in the action bar are rerendered in every frame when brushing in a volume tracing. I originally noticed this, because I thought brushing felt a little sluggish and then used the React Dev Tools - Highlight Updates to visualize. We should optimize this in another PR :)

frontend/javascripts/navbar.js Outdated Show resolved Hide resolved
frontend/javascripts/navbar.js Outdated Show resolved Hide resolved
frontend/javascripts/navbar.js Outdated Show resolved Hide resolved
frontend/javascripts/navbar.js Outdated Show resolved Hide resolved
frontend/stylesheets/trace_view/_tracing_view.less Outdated Show resolved Hide resolved
@philippotto
Copy link
Member Author

Thanks for your thorough feedback! I think the PR is good to go now. Will probably merge tomorrow morning.

@philippotto
Copy link
Member Author

@daniel-wer If you give your official approval, I'd merge this :)

@philippotto philippotto merged commit bceec98 into master Mar 20, 2019
@normanrz normanrz deleted the embeddable-wk branch August 12, 2019 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants