Skip to content

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

Merged
Gaurav0 merged 4 commits intoSeeClickFix:masterfrom
Gaurav0:use_postmessage_instead_of_storage_event_embedded_case
Sep 23, 2024
Merged

[TP-179336] Use postmessage instead of storage event for embedded case#2
Gaurav0 merged 4 commits intoSeeClickFix:masterfrom
Gaurav0:use_postmessage_instead_of_storage_event_embedded_case

Conversation

@Gaurav0
Copy link
Copy Markdown

@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
Comment thread addon/mixins/ui-service-mixin.js
@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
Copy Markdown

@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
Copy Markdown

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
Copy Markdown
Author

Gaurav0 commented Sep 20, 2024

@AndrewHYi Tests are running automatically.

@AndrewHYi
Copy link
Copy Markdown

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') {
Copy link
Copy Markdown

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).

Comment thread package.json
{
"name": "torii",
"version": "1.0.0-beta.2",
"version": "1.0.0-beta.3",
Copy link
Copy Markdown

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
Copy Markdown

@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
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