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

[TP-179336] Use postmessage instead of storage event for embedded case #2

Conversation

Gaurav0
Copy link

@Gaurav0 Gaurav0 commented Aug 20, 2024

No description provided.

@Gaurav0 Gaurav0 requested a review from AndrewHYi August 20, 2024 20:48
@Gaurav0 Gaurav0 self-assigned this Aug 20, 2024
@Gaurav0 Gaurav0 force-pushed the use_postmessage_instead_of_storage_event_embedded_case branch 5 times, most recently from 61a4f76 to 0d44b14 Compare August 23, 2024 12:14
@Gaurav0 Gaurav0 marked this pull request as ready for review August 23, 2024 12:26
@Gaurav0 Gaurav0 force-pushed the use_postmessage_instead_of_storage_event_embedded_case branch from 0d44b14 to 03437d6 Compare August 23, 2024 18:08
@AndrewHYi AndrewHYi requested a review from a team September 3, 2024 18:19
@Gaurav0 Gaurav0 force-pushed the use_postmessage_instead_of_storage_event_embedded_case branch from a53f9d9 to 35563b2 Compare September 17, 2024 14:20
@Gaurav0 Gaurav0 requested a review from AndrewHYi September 19, 2024 20:14
Copy link

@AndrewHYi AndrewHYi left a comment

Choose a reason for hiding this comment

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

suggestion: Is there a way we can run/update the torii tests?

@zbeat zbeat changed the title Use postmessage instead of storage event for embedded case [TP-179336] Use postmessage instead of storage event for embedded case Sep 20, 2024
@zbeat
Copy link
Member

zbeat commented Sep 20, 2024

TP story link:
https://civicplus.tpondemand.com/entity/179336-open-new-window-vs-iframe-modal

We need to fix the TP<>Github integration permissions, so all SCF repos are included.

@Gaurav0
Copy link
Author

Gaurav0 commented Sep 20, 2024

@AndrewHYi Tests are running automatically.

@AndrewHYi
Copy link

AndrewHYi commented Sep 20, 2024

@Gaurav0 nice, looks like existing tests pass - should we add tests for the new scenarios?

@Gaurav0 Gaurav0 requested a review from AndrewHYi September 23, 2024 15:07
@AndrewHYi AndrewHYi requested a review from a team September 23, 2024 15:08
resolve(data);
});
}
if (messageEvent.data === 'getPendingRequestKey') {

Choose a reason for hiding this comment

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

suggestion: this fix should be merged alongside the first commit. Also, the commit about feature flags is confusing here, but that only makes sense in the context of the SCF application (not here in Torii fork land).

@@ -1,6 +1,6 @@
{
"name": "torii",
"version": "1.0.0-beta.2",
"version": "1.0.0-beta.3",

Choose a reason for hiding this comment

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

suggestion: the version bump should be the last commit - also, do we need to bump the version up again?

@@ -0,0 +1,127 @@
/* eslint-disable ember/no-mixins, ember/no-new-mixins, ember/no-classic-classes */

import EmberObject from '@ember/object';
Copy link

@AndrewHYi AndrewHYi Sep 23, 2024

Choose a reason for hiding this comment

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

suggestion: let's add the test to the first commit.

thought: I need some help walking through the test code - I'll schedule something for this afternoon.

Update: tests seem to be failing - can reschedule the group review we rescheduled if you need more time.

@Gaurav0 Gaurav0 force-pushed the use_postmessage_instead_of_storage_event_embedded_case branch from 0442fa2 to 9332403 Compare September 23, 2024 16:04
@Gaurav0 Gaurav0 force-pushed the use_postmessage_instead_of_storage_event_embedded_case branch from 9332403 to 154ec01 Compare September 23, 2024 17:04
@Gaurav0 Gaurav0 merged commit 1ec0dde into SeeClickFix:master Sep 23, 2024
12 checks passed
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.

3 participants