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.

Sign up for GitHub

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

Jump to bottom

Refactor InputLookupQA to use React hooks #1311

Merged
merged 2 commits into from Sep 19, 2019
Merged

Refactor InputLookupQA to use React hooks #1311

merged 2 commits into from Sep 19, 2019

Conversation

jermnelson
Copy link
Contributor

@jermnelson jermnelson commented Sep 13, 2019 •
edited

This PR fixes open issue #1273, refactored InputLookupQA to use React hooks for retrieving options similar to the InputLookupSinopia pattern.

Remaining work before this draft PR is finished:

  • Fix tests and eslint errors and warnings
  • Support subtype=context

@jermnelson jermnelson force-pushed the embedded-qa-fix branch 4 times, most recently from 3e02bfe to 04e428b Compare September 17, 2019 06:31
@jermnelson jermnelson marked this pull request as ready for review September 17, 2019 06:36
justinlittman
justinlittman reviewed Sep 17, 2019
View reviewed changes
src/components/editor/property/InputLookupQA.jsx Outdated
import { bindActionCreators } from 'redux'
import { connect } from 'react-redux'
import {
itemsForProperty, getDisplayValidations, getPropertyTemplate, findErrors,
} from 'selectors/resourceSelectors'
import { changeSelections } from 'actions/index'
import search from 'actionCreators/qa'
import { getSearchResults } from 'actionCreators/qa'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the switch from search to getSearchResults?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactoring based on pairing with @justinlittman, using getSearchResults from a new qa.js module in utilities.

src/components/editor/property/InputLookupQA.jsx

const search = (query) => {
const currentAuthorities = []
const resultPromise = getSearchResults(query, props.propertyTemplate)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh. I looked at getSearchResults -- we use a Promise.all(), which means we wait for the slowest search. Are we sure we want to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not the greatest but I think speeding up/refactoring so that we are not using Promise.all is a refactoring work. The QA search speed on the Sinopia side hasn't changed with this PR.

@jermnelson jermnelson force-pushed the embedded-qa-fix branch 2 times, most recently from c63f8a6 to 01f7978 Compare September 17, 2019 14:51
@justinlittman
Copy link
Contributor

@jermnelson Is there a resource template that replicates the problem in local dev?

@jermnelson
Copy link
Contributor Author

@jermnelson Is there a resource template that replicates the problem in local dev?

Yes, in the Harvard:rt:test:userfeedback RT, click on Step 5 (add an instance and an item. Use Preview RDF to confirm that your work, instance, and item are linked to each other as expected.), add an Instance and then add Publication, Distribution, Manufacture Statements, click on Place, and then the Add Place demonstrates the bug.

@justinlittman
Copy link
Contributor

justinlittman commented Sep 18, 2019 •
edited

A warning:
Screen Shot 2019-09-18 at 3 51 41 PM

@justinlittman
Copy link
Contributor

And an error:
Screen Shot 2019-09-18 at 3 53 23 PM

Screen Shot 2019-09-18 at 3 55 31 PM

@justinlittman
Copy link
Contributor

Steps to reproduce (running locally):

  1. Select Sinopia user feedback rt.
  2. Click +Add for 5.
  3. Click +Add Publication, Distribution, Manufacture Statements.
  4. Toggle open.
  5. Click +Add Name for Publication Activity.
  6. Click +Add Search Name.
  7. Enter "justin".
  8. Wait.

@jermnelson
Copy link
Contributor Author

Hi @justinlittman, good catch. I just added some error checking if QA returns an error. The Add Name uses urn:ld4p:qa:names:organization QA authority that is returning an error.

@justinlittman
Copy link
Contributor

The error message needs some cleanup:
Screen Shot 2019-09-19 at 7 54 51 AM

After that I'd say, let's merge and get some users to test on development env.

@jermnelson
Copy link
Contributor Author

Done, error message is now displayed as:
Screen Shot 2019-09-19 at 2 51 10 PM

@jermnelson
Refactor InputLookupQA to use React hooks, fixes bug #1273, still nee…
27e2f60 
…d to refactor Menu function
@jermnelson
Moved and refactored qa.js from actionCreators to utilities to
f1b0197 
better reflect that we are no longer using an
actionCreator in the InputLookupQA component
@jermnelson jermnelson merged commit 4ab25fb into master Sep 19, 2019
@jermnelson jermnelson deleted the embedded-qa-fix branch September 19, 2019 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinlittman justinlittman justinlittman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jermnelson @justinlittman