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

Stop using react-iframe library in dashboard #3695

Merged
merged 1 commit into from Nov 25, 2019

Conversation

@cynthia-sg
Copy link
Collaborator

cynthia-sg commented Nov 11, 2019

react-iframe library is only rendering a normal iframe, so we can delete it.

Closes #2915

Signed-off-by: Cintia Sanchez Garcia cynthiasg@icloud.com

@cynthia-sg cynthia-sg self-assigned this Nov 11, 2019
@cynthia-sg cynthia-sg added the area/web label Nov 11, 2019
@cynthia-sg cynthia-sg requested a review from scottcarol Nov 11, 2019
Copy link
Member

scottcarol left a comment

Thanks for taking this on, I'm all for removing dependencies! 👍

Unlike the master branch, this PR shows a scrollbar for the internal iframe, which is not desired -- the goal is for the iframe to not obviously be an iframe, and to blend in with the other content of the page. Can this PR be updated to hide the scrollbar but still show the complete contents of the Community page, in Chrome/Safari/FireFox?

Master:
Screen Shot 2019-11-12 at 7 43 42 AM

This PR:
Screen Shot 2019-11-12 at 7 45 38 AM

@cynthia-sg

This comment has been minimized.

Copy link
Collaborator Author

cynthia-sg commented Nov 12, 2019

Can this PR be updated to hide the scrollbar but still show the complete contents of the Community page, in Chrome/Safari/FireFox?

The iframe scrollbar is now hidden and the page looks like master. Thanks

@scottcarol

This comment has been minimized.

Copy link
Member

scottcarol commented Nov 12, 2019

Thanks for updating this! When testing it out on my browser, it looks like the browser cuts off the bottom of the posts if my browser height is shorter than the height of the iframe. It also looks like this was happening beforehand - you didn't introduce this! We didn't catch it initially because when we introduced the feature there was only one community update post to start.

Screen Shot 2019-11-12 at 10 17 13 AM

Is there a way to have the complete iframe content show in the page, with no scrollbar? I'd really prefer not to have an extra scrollbar. If it's there's not a clear solution, we can merge this fix in and create a follow-up issue.

@cynthia-sg

This comment has been minimized.

Copy link
Collaborator Author

cynthia-sg commented Nov 12, 2019

Is there a way to have the complete iframe content show in the page, with no scrollbar? I'd really prefer not to have an extra scrollbar. If it's there's not a clear solution, we can merge this fix in and create a follow-up issue.

I have been checking a way to get iframe height but due to cross origin issues we don’t have access to contentWindow to get scrollHeight, so... I agree about merging this PR for now.

@tegioz tegioz added this to In Progress in L5D-Dashboard-EVOL via automation Nov 14, 2019
@tegioz tegioz moved this from In Progress to Pending Reviewer Approval in L5D-Dashboard-EVOL Nov 14, 2019
@scottcarol

This comment has been minimized.

Copy link
Member

scottcarol commented Nov 15, 2019

@cynthia-sg Sounds good, this PR has a benefit on its own since it gets rid of a dependency, and doesn't break any additional functionality. Could you make a follow-up issue for the cut-off posts? There may be something we can do on the linkerd.io side as well.

@scottcarol

This comment has been minimized.

Copy link
Member

scottcarol commented Nov 15, 2019

Oops looks like there is a merge issue because our yarn lockfile changed yesterday. Would you mind rebasing to include the latest yarn.lock?

@scottcarol scottcarol self-requested a review Nov 15, 2019
Copy link
Member

scottcarol left a comment

Thanks for removing this dependency!

Signed-off-by: Cintia Sanchez Garcia <cynthiasg@icloud.com>
@cynthia-sg cynthia-sg force-pushed the cynthia-sg/remove-react-iframe branch from 2fa79c0 to fde08cc Nov 18, 2019
@cynthia-sg

This comment has been minimized.

Copy link
Collaborator Author

cynthia-sg commented Nov 18, 2019

Oops looks like there is a merge issue because our yarn lockfile changed yesterday. Would you mind rebasing to include the latest yarn.lock?

Done!

Copy link
Member

dadjeibaah left a comment

This changes looks good to me! Thanks for making this update 📦 🚢

L5D-Dashboard-EVOL automation moved this from Pending Reviewer Approval to Approved Nov 25, 2019
@scottcarol

This comment has been minimized.

Copy link
Member

scottcarol commented Nov 25, 2019

Great, merging now. @cynthia-sg Could you make a follow-up issue for the Community page cut-off issue, as discussed earlier in this PR? You don't have to assign it to yourself, but we should make a note of it and come up with a plan to fix.

@scottcarol scottcarol merged commit 65d5778 into master Nov 25, 2019
28 checks passed
28 checks passed
Checkout source
Details
Validate go deps
Details
Go unit tests
Details
Go lint
Details
Go Format
Details
JS unit tests
Details
Docker pull
Details
Docker build
Details
Cluster setup (deep)
Details
Cluster setup (upgrade)
Details
Cluster setup (helm)
Details
Cluster setup (custom_domain)
Details
Cluster setup (external_issuer)
Details
Integration tests (deep)
Details
Integration tests (upgrade)
Details
Integration tests (helm)
Details
Integration tests (custom_domain)
Details
Integration tests (external_issuer)
Details
Cluster cleanup (deep)
Details
Cluster cleanup (upgrade)
Details
Cluster cleanup (helm)
Details
Cluster cleanup (custom_domain)
Details
Cluster cleanup (external_issuer)
Details
Docker deploy
Details
Cloud integration tests
Details
Cloud cleanup
Details
Helm chart deploy
Details
DCO DCO
Details
L5D-Dashboard-EVOL automation moved this from Approved to Done Nov 25, 2019
@scottcarol scottcarol deleted the cynthia-sg/remove-react-iframe branch Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.