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

[dataquery] Add ability for other modules to add actions #9302

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

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Jun 20, 2024

This adds the ability for other modules to add custom actions to the "Next Steps" of the dataquery module. An example of an "Action" that might be added is a "Package Data Release" for the data_release module or a "Send to CBRAIN" for a theoretical CBRAIN module on the final view data page.

To add an action, the module's getWidgets must return an array of \LORIS\dataquery\ActionWidgets to be added when called with the type argument of dataquery:action. Each ActionWidget adds a button on a particular dataquery page which, when clicked, invokes a javascript callback that can be customized by the module.

Plugin actions provided by a module are displayed in a second row underneath the "Next Steps" .

@driusan driusan added Feature PR or issue introducing/requiring at least one new feature UI PR or issue introducing/requiring improvements to the LORIS User Interface javascript Pull requests that update Javascript code labels Jun 20, 2024
Copy link
Contributor

@maximemulder maximemulder left a comment

Choose a reason for hiding this comment

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

I don't know anything about the dataquery module or its code logic, so here is my very surface-level review.

Comment on lines 56 to 61
steps.push(<ButtonElement
label={fieldLabel}
columnSize='col-sm-12'
key='fields'
onUserInput={() => props.changePage('DefineFields')}
onUserInput={() => props.changePage(Tabs.Fields)}
/>);
Copy link
Contributor

Choose a reason for hiding this comment

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

This button element seems to be repeated a lot, can it be factorized ?

Comment on lines +60 to +66
if (steps[Tabs.Info]) {
steps[Tabs.Info].push({
label: widget.label,
action: func,
});
}
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

The pattern of this switch should maybe be factorized ?

Comment on lines +36 to +38
if (activeTab == Tabs.Fields
|| activeTab == Tabs.Filters
|| activeTab == Tabs.Data) {
Copy link
Contributor

@maximemulder maximemulder Jul 2, 2024

Choose a reason for hiding this comment

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

Could you use strict equality here (and in a few other places) ?

function getCallback(name: string): (() => void) | null {
const namepieces = name.split('.').reverse();
let level: any = window;
for (let name = namepieces.pop(); name; name = namepieces.pop()) {
Copy link
Contributor

@maximemulder maximemulder Jul 2, 2024

Choose a reason for hiding this comment

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

Could you use for of here ?

/>);
break;
}

for (const osteps of (props.extrasteps[props.page] || [])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd personally put the for inside a if instead.

Comment on lines +40 to +41


Copy link
Contributor

Choose a reason for hiding this comment

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

Extra line breaks (we should probably add a lint for that).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature PR or issue introducing/requiring at least one new feature javascript Pull requests that update Javascript code UI PR or issue introducing/requiring improvements to the LORIS User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants