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

Prevent upload modal & flash message from showing stale messages #550

Merged
merged 1 commit into from
May 23, 2019

Conversation

mjgiarlo
Copy link
Member

Fixes #534

This branch includes the following changes:

  • Reset messages and errors in ImportResourceTemplate component when HTTP response is successful
  • Reset messages when update handler is finished
  • Prevent users from dismissing flash message box, else it never comes back
  • Extract code to parse a response object into a human-friendly location into a new function in ImportResourceTemplate component
  • Add Babel plugin for optional chaining (as dev dependency) via the safe navigation (or "Elvis") operator
    • This lets us safely reach into objects such as HTTP responses without having to check at every level for null or undefined

@mjgiarlo mjgiarlo added the M3 label May 23, 2019
@mjgiarlo mjgiarlo added this to Needs Review in Sinopia Work-Cycle One via automation May 23, 2019
@coveralls
Copy link

coveralls commented May 23, 2019

Coverage Status

Coverage decreased (-1.1%) to 79.024% when pulling afc9e90 on refresh_rt_flash_messages_534 into deee924 on master.

Copy link
Contributor

@jermnelson jermnelson left a comment

Choose a reason for hiding this comment

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

LGTM; just a couple of informational questions

package.json Show resolved Hide resolved
Copy link
Contributor

@ndushay ndushay left a comment

Choose a reason for hiding this comment

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

minor quibbles about some test data naming and what I feel are some superfluous comments (I'm hearing Sandi Metz 'if you read the code, what would you say out loud?' and if it matches the comment, the comment can go?)

__tests__/components/editor/ImportResourceTemplate.test.js Outdated Show resolved Hide resolved
__tests__/components/editor/ImportResourceTemplate.test.js Outdated Show resolved Hide resolved
src/components/editor/ImportResourceTemplate.jsx Outdated Show resolved Hide resolved
src/components/editor/ImportResourceTemplate.jsx Outdated Show resolved Hide resolved
src/components/editor/ImportResourceTemplate.jsx Outdated Show resolved Hide resolved
src/components/editor/ImportResourceTemplate.jsx Outdated Show resolved Hide resolved
this.setState(newState)
}

// Returns a URL or an empty string
humanReadableLocation = response => {
Copy link
Contributor

Choose a reason for hiding this comment

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

like this method naming.

@ndushay
Copy link
Contributor

ndushay commented May 23, 2019

If you're in a rush to merge, I can change to "approve" but I think the cleanup would help readability of tests and code.

Fixes #534

This branch includes the following changes:
* Reset messages and errors in `ImportResourceTemplate` component when HTTP response is successful
* Reset messages when update handler is finished
* Prevent users from dismissing flash message box, else it never comes back
* Extract code to parse a response object into a human-friendly location into a new function in `ImportResourceTemplate` component
* Add Babel plugin for optional chaining (as dev dependency) via the safe navigation (or "Elvis") operator
  * This lets us safely reach into objects such as HTTP responses without having to check at every level for `null` or `undefined`
@mjgiarlo mjgiarlo force-pushed the refresh_rt_flash_messages_534 branch from 96566df to afc9e90 Compare May 23, 2019 18:50
Sinopia Work-Cycle One automation moved this from Needs Review to In Progress May 23, 2019
@ndushay ndushay merged commit 4f8a5f2 into master May 23, 2019
Sinopia Work-Cycle One automation moved this from In Progress to Done May 23, 2019
@mjgiarlo mjgiarlo deleted the refresh_rt_flash_messages_534 branch May 23, 2019 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

strangeness with warning and confirmation messages on RT upload
4 participants