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

Allow the parent window to configure and manage navigation requests #224

Merged

Conversation

cstns
Copy link
Contributor

@cstns cstns commented Apr 8, 2024

Allow the parent window to configure and manage navigation requests when the NR editor resides in an embedded context/iframe

Description

Related Issue(s)

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

@closes #223

…hen the NR editor resides in an embedded context/iframe
@cstns
Copy link
Contributor Author

cstns commented Apr 8, 2024

Some feedback / input would be appreciated on the interception of the logo click event

@cstns cstns changed the title Allow the parent window to configure and manage navigation requests w… Allow the parent window to configure and manage navigation requests Apr 8, 2024
@cstns cstns requested a review from Steve-Mcl April 8, 2024 14:54
@cstns cstns self-assigned this Apr 8, 2024
package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

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

I think the inner package for the theme plugin should be bumped

https://github.com/FlowFuse/nr-launcher/blob/d3429f125e13dc78d3fcba5151d246160e3732ee/lib/theme/package.json#L3C5-L3C24

@hardillb do you see any technical reason not to bump theme in the inner package.json in this PR?

@hardillb
Copy link
Contributor

hardillb commented Apr 8, 2024

If we are confident that this is it for changes to the theme, then yes, bump the inner package.json.

@sherbastian The top level nr-launcher package.json will get bumped by the release script I mentioned on our call earlier. I will try to remember to point it out in the release huddle on Thursday.

lib/theme/common/forge-common.js Show resolved Hide resolved
lib/theme/common/forge-common.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@cstns
Copy link
Contributor Author

cstns commented Apr 16, 2024

Reverted the v bump, the PR should be good to go. It can be released independently of #3656 without any side effects

Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

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

All good @sherbastian except I beleive we should be updating the internal theme package.json to 1.9.0

@cstns
Copy link
Contributor Author

cstns commented Apr 17, 2024

@Steve-Mcl, should I do it manually?

@Steve-Mcl
Copy link
Contributor

@Steve-Mcl, should I do it manually?

Yes, just update the value in this PR. The inner package is not published and has no automations.

@cstns cstns requested a review from Steve-Mcl April 17, 2024 07:56
Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

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

Pulled and tested locally against current FF core. Working as expected. Happy to approve.

I won't merge as I don't know the status of the other parts of this puzzle (so just in case any tweaks are needed across the PRs). I'll leave it to you to merge

You can merge at will.

@cstns cstns merged commit 6e36168 into main Apr 18, 2024
5 checks passed
@cstns cstns deleted the 223-two-way-communication-between-iframe-and-parent-window branch April 18, 2024 11:40
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.

Allow 2-way communication between the parent window and embedded NR instances
3 participants