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

Make window.kolibri object available to Hashi iframe context #7942

Merged

Conversation

marcellamaki
Copy link
Member

This is very much a draft/WIP PR that is working to establish the foundation for a kolibri object and the core MVP functions.

Some functions are fully working, some are WIP with elements of pseudo code, as I need a more well-thought-out strategy for doing manual QA in some places. The new data-fetching code in mainClient.js should most likely be relocated to another file - maybe a handlers.js, or within the custom channels vue file which is not yet finished (TBD).

I am also not sure if __navigateTo() is doing enough, and if it's not, what exactly needs to be adjusted/added.

@marcellamaki marcellamaki added the work-in-progress Not ready for review label Apr 1, 2021
@marcellamaki marcellamaki marked this pull request as draft April 1, 2021 22:39
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

All the basics seem to be in place. One thing I would strongly urge you to consider sooner rather than later is getting some automated tests in place - the existing hashi tests should give some guidance on how to best go about that.

} else if (options.parent) {
getParams.parent = options.parent;
}
options.ids ? (getParams.ids = options.ids) : null;
Copy link
Member

Choose a reason for hiding this comment

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

It's purely personal preference, but I find this in-line ternary operator to conditionalize assignment a bit hard to read.

You could also achieve the effect you want here by using the lodash helper function pick https://lodash.com/docs/4.17.15#pick

const getParams = pick(options, ['ids' ,'page', 'pageSize', 'parent']);
if (getParams.parent === 'self') {
...
}

Pick will only assign defined properties.


// if filtering by optional params
if (message.options) {
let getParams = {};
Copy link
Member

Choose a reason for hiding this comment

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

Can use const for both getParams and options here as you are never reassigning their values. You've assigned it to an object, and even though you are changing the attributes of that object, you are not changing the referent of the variable.

packages/hashi/src/mainClient.js Outdated Show resolved Hide resolved
packages/hashi/src/mainClient.js Outdated Show resolved Hide resolved
packages/hashi/src/mainClient.js Outdated Show resolved Hide resolved
}

__getOrUpdateContext(message) {
// to update context with the incoming context
Copy link
Member

Choose a reason for hiding this comment

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

Note we need to do the URI encoding and decoding of the context here.

packages/hashi/src/mainClient.js Outdated Show resolved Hide resolved
@@ -50,6 +52,37 @@ class Mediator {
this.remote.postMessage(message, '*');
}

// a function to manage messages for kolibri.js,
// when most messages require a response, to minimize redundancy
sendMessageAwaitReply({ event, data, nameSpace }) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks very familiar....

return new Promise((resolve, reject) => {
const msgId = uuidv4();
function handler(message) {
if (message.message_id === msgId && message.type === 'response') {
Copy link
Member

Choose a reason for hiding this comment

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

Once we've received the corresponding message, would be good to unregister the message handler for cleanup.

@@ -59,6 +63,18 @@ export default class MainClient {
});
});
this.mediator.sendMessage({ nameSpace, event: events.READYCHECK, data: true });

this.on(this.events.DATAREQUESTED, message => {
this.__fetchContentData(message);
Copy link
Member

Choose a reason for hiding this comment

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

I am surprised you have not needed to call .bind on any of the handlers you have here.

@marcellamaki marcellamaki force-pushed the custom-channels-working-branch branch from 55ba11f to 549a610 Compare April 9, 2021 15:09
@marcellamaki marcellamaki force-pushed the custom-channels-working-branch branch from 549a610 to 3d3b06b Compare April 9, 2021 17:39
@jonboiser jonboiser self-assigned this Apr 13, 2021
@marcellamaki marcellamaki force-pushed the custom-channels-working-branch branch from b93ba75 to 1afd9dc Compare April 13, 2021 21:56
@marcellamaki marcellamaki added TODO: needs review Waiting for review and removed work-in-progress Not ready for review labels Apr 13, 2021
@marcellamaki marcellamaki marked this pull request as ready for review April 13, 2021 22:00
@jonboiser jonboiser added this to the 0.15.0 milestone Apr 14, 2021
@jonboiser jonboiser removed the TODO: needs review Waiting for review label Apr 15, 2021
@jonboiser jonboiser changed the title Work in Progress - Custom channels working branch Make window.kolibri object available to Hashi iframe context Apr 15, 2021
@jonboiser jonboiser merged commit b60585f into learningequality:develop Apr 15, 2021
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.

None yet

3 participants