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

Fix double-trigger of reload in OSCALLoaderForm #491

Merged
merged 3 commits into from
Aug 1, 2022

Conversation

kylelaker
Copy link
Contributor

@kylelaker kylelaker commented Jul 10, 2022

Currently, the onUrlChange() is fired whenever any character is typed.
This is sorta undesirable because it can result in various re-rendering
happening in a few places prematurely.

As-is, this doesn't work. It seems like the actual value received by
OSCALLoader "lags" behind the input value by one form submission.

Relies on #490

@kylelaker kylelaker requested a review from tuckerzp July 11, 2022 16:02
Currently, the `onUrlChange()` is fired whenever any character is typed.
This is sorta undesirable because it can result in various re-rendering
happening in a few places prematurely.

As-is, this doesn't work. It seems like the actual value received by
`OSCALLoader` "lags" behind the input value by one form submission.
@kylelaker kylelaker force-pushed the feature/make-form-behave-predictably branch from 5943cb9 to 822aab6 Compare July 11, 2022 18:20
@Bronstrom
Copy link
Contributor

Relies on #490

With 490 merged, I believe this can be addressed.

I'm glad that were handling this now! I noticed a major slowdown here too when adjusting the form URL, and was thinking it was related to the onChange event. Looking over the code limiting the amount of re-rendering and testing the branch, it appears everything works well.

@tuckerzp
Copy link
Contributor

tuckerzp commented Jul 29, 2022

It looks like the oscalUrl is not actually being changed to the new value here?

https://github.com/EasyDynamics/oscal-react-library/blob/feature/make-form-behave-predictably/src/components/OSCALLoader.js#L130-#L132

The State was not being updated quickly enough before reloading the
document. This led to it seeming like the Viewer was lagging behind by
one each time you hit `Reload`. Instead, the `LoaderForm` merely handles
changing the current oscalUrl and the Loader reloadings upon that
change.
@tuckerzp
Copy link
Contributor

I will wait for @kylelaker to review my change and take it out of draft, but I think this should be fixed. There was a weird issue where we were reloading before the state had a change to actually update.

@kylelaker kylelaker marked this pull request as ready for review July 29, 2022 18:13
@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team July 29, 2022 18:13
@kylelaker
Copy link
Contributor Author

Thanks for helping with this @tuckerzp. I approve of the changes you've made here; I think you should feel free to mark this PR as approved as well (if you do approve).

@easy-dynamics-oscal-automation easy-dynamics-oscal-automation bot requested a review from a team August 1, 2022 13:43
@mikeisen1 mikeisen1 merged commit 353d222 into develop Aug 1, 2022
@mikeisen1 mikeisen1 deleted the feature/make-form-behave-predictably branch August 1, 2022 14:03
@kylelaker kylelaker changed the title Try to improve OSCALLoaderForm structure Fix double-trigger of reload in OSCALLoaderForm Aug 29, 2022
@kylelaker kylelaker added the bug Something isn't working label Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants