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

[Instrument manager] Reactify Menu Filters #4142

Merged
merged 9 commits into from
Nov 23, 2018

Conversation

xlecours
Copy link
Contributor

Brief summary of changes

Now use react for the menu filter.
Adding filters

Move the upload form in a different table.
Use ajax to upload the instrument.
Adds handler for POST requests in the php file.

This resolves issue...

This module was slowly drifting into the abyss

To test this change...

Please run the test plan.

@xlecours xlecours changed the base branch from minor to major November 22, 2018 03:45
@xlecours xlecours added Area: UI PR or issue related to the user interface Cleanup PR or issue introducing/requiring at least one clean-up operation [branch] major labels Nov 22, 2018
@xlecours xlecours mentioned this pull request Nov 22, 2018
Copy link
Collaborator

@HenriRabalais HenriRabalais left a comment

Choose a reason for hiding this comment

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

Took a first glance at the JSX. Looks good! Just a few suggestions and clarifications to address.

modules/instrument_manager/jsx/instrumentManagerIndex.js Outdated Show resolved Hide resolved
modules/instrument_manager/jsx/instrumentManagerIndex.js Outdated Show resolved Hide resolved
];

const feedback = () => {
if (!this.state.data.caninstall) {
Copy link
Collaborator

@HenriRabalais HenriRabalais Nov 22, 2018

Choose a reason for hiding this comment

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

Perhaps canInstall should be passed in a permission object? It just seems to odd to have it as part of 'data'. This may require some restructuring of how we handle permissions on the front end. Is there a way to access this information via this.props.hasPermission?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think so. Whether or not they can install is a function of the filesystem permissions/setup, not the user's permission.

Copy link
Contributor Author

@xlecours xlecours Nov 22, 2018

Choose a reason for hiding this comment

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

I partly agree with you. Yes, it is odd the it is part of the data but it is not a permission either. This tells if quat user is configured correctly so the table can be created.

Copy link
Collaborator

@HenriRabalais HenriRabalais Nov 22, 2018

Choose a reason for hiding this comment

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

@driusan @xlecours Hmm, okay. Yeah, that makes. I guess as we are making standards for the objects we are passing to the front end (e.g. data, fieldOptions, etc.), we may want to group things like this into a configuration object? Rather than have stray configuration booleans in the data object. Not necessary for this PR, just something to think about.

modules/instrument_manager/jsx/instrumentManagerIndex.js Outdated Show resolved Hide resolved

const uploadTab = () => {
let content = null;
if (this.state.data.writable) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same concern as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same,this tells if the project/instrument and project/table_sql directory can be written into.

modules/instrument_manager/jsx/uploadForm.js Show resolved Hide resolved
modules/instrument_manager/jsx/uploadForm.js Outdated Show resolved Hide resolved
@driusan
Copy link
Collaborator

driusan commented Nov 22, 2018

@xlecours failing Travis

@xlecours
Copy link
Contributor Author

@driusan Passing now

@driusan driusan merged commit dcb1e1b into aces:major Nov 23, 2018
driusan pushed a commit that referenced this pull request Nov 27, 2018
Remove the function that counts number of pages. It was used for Pagination of menu filters, which is now done in React.

#4142 removes the last usage of it.
@ridz1208 ridz1208 added this to the 21.0.0 milestone Dec 3, 2018
@xlecours xlecours deleted the remove_totalitems branch August 4, 2021 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: UI PR or issue related to the user interface Cleanup PR or issue introducing/requiring at least one clean-up operation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants