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

adding advertisement tile for the new browser ui #1978

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nnecla
Copy link
Contributor

@nnecla nnecla commented Oct 10, 2024

Description

This pr is to add the PreviewFrame which contains the advertisement tile for the new browser UI. This frame will only be shown if the new UI is there, and the feature flag is on.

Also, if user prefers the new UI, we switch to the new UI before the app renders.

The look of the frame is not final and wip.

@nnecla nnecla requested a review from a team October 10, 2024 21:32
@nnecla
Copy link
Contributor Author

nnecla commented Oct 10, 2024

add tile in :play start

@nnecla
Copy link
Contributor Author

nnecla commented Oct 10, 2024

;(async () => {
const doesPreferQuery = localStorage.getItem('prefersOldBrowser') === 'false'

const response = await fetch('/preview/manifest.json')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think fetch can throw, so I think it'd be good with try { } catch { } here

Copy link
Contributor

Choose a reason for hiding this comment

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

This also needs to be fetch('./preview/manifest.json') to handle the bundled case where it's under /browser/preview. And also to handle if users have set up their own custom hosting!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect point, changed it accordingly!

const response = await fetch('/preview/manifest.json')
if (response.status === 200) {
if (doesPreferQuery) {
window.location.pathname = '/preview/'
Copy link
Contributor

Choose a reason for hiding this comment

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

In the bundled Browser case, the preview will be on http://localhost:7474/browser/preview and in the hosted case it's on browser.neo4j.io/preview.

So this would not work for bundled browser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is so true. I've also realised while testing that switching back to browser removes the whole path, instead of just removing the preview(i remember i've tested it but probably did not test it appropriately :/ ) But I can create another pr to fix the query side.

Changed this one to add preview to the current path.

Comment on lines 21 to 27
[experimentalFeaturePreviewName]: {
name: experimentalFeaturePreviewName,
on: false,
displayName: 'Advertise preview of new browser',
tooltip: 'Enable the advertisement tile of the new browser'
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a user facing feature flag, I think we're better off to just have it on always if isPreviewAvailable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it!

Copy link
Contributor

@OskarDamkjaer OskarDamkjaer left a comment

Choose a reason for hiding this comment

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

Cool stuff! 💯 I think it'd be good to build the .jar file and use that for our bundled browser testing in the query side, then we can do an e2e for the full flow

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.

2 participants