-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat/image-quality-playback #1161
Conversation
export function FileUploadFieldEdit() { | ||
const { state, dispatch } = useContext(ComponentContext); | ||
const { selectedComponent } = state; | ||
const { options = {} } = selectedComponent; | ||
const { options = defaultOptions } = selectedComponent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd do
const options = { ...defaultOptions, ...selectedComponent.options }
e.g. if the default options were (truthy values is a better example)
{
multiple: true,
imageQualityPlayback: true,
}
and someone configured only
{
multiple: false
}
you'd expect the options to be
{
multiple: false,
imageQualityPlayback: true,
}
but since options are no longer undefined, const { options = defaultOptions } = selectedComponent
, = defaultOptions
won't be applied, and imageQualityPlayback
will evaluate to falsy in this case.
It's fine in this case since the defaults are both falsy, but when you have more options or options with a mixture of defined/truthy values this wont work as intended.
savedState | ||
); | ||
|
||
if (request?.pre?.warningFromApi === "qualityWarning") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this set? should the pre handler just do a response takeover and return the response itself?
06477e6
to
ab3aab9
Compare
viewModel.backLink = progress[progress.length - 2]; | ||
viewModel.backLink = progress[progress.length - 2] ?? this.backLinkFallback; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to declare twice?
let error: string | undefined; | ||
let location: string | undefined; | ||
console.log("This is the payload", warningFromApi); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log
@@ -16,6 +16,8 @@ export function optionsReducer(state, action: OptionsActions) { | |||
const { type, payload } = action; | |||
const { selectedComponent } = state; | |||
const { options } = selectedComponent; | |||
console.log("reducer type: ", type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logs
} | ||
|
||
buildRadioViewModel(error?: string) { | ||
const standardViewModel = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably set this to the class itself, otherwise it's going to create this object every time buildRadioViewModel is called
@@ -95,6 +95,7 @@ export class UploadService { | |||
return { | |||
location, | |||
error, | |||
warningFromApi, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fromApi is already implied for the other values, just warning
is fine
… handler after checking whether a redirect is needed
…onses in upload service
…troller is being used
eadc583
to
a72660a
Compare
import { HapiRequest, HapiResponseToolkit } from "server/types"; | ||
export class PlaybackUploadPageController extends PageController { | ||
inputComponent: FormComponent; | ||
standardViewModel = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
retryUploadViewModel
- standardViewModel/standard by itself doesn't convey enough information
const state = await cacheService.getState(request); | ||
const { progress = [] } = state; | ||
const payload = request.payload; | ||
if (!payload.retryUpload) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do a joi validation here?
… summary page controller
Description
With new support for image quality checking for file upload, research suggests that users don't respond well to hard stops. To mitigate this, this feature implements a playback screen so the user has autonomy in deciding whether they want to continue and risk delaying their application or not.
How to enable
To use the new upload page controller, add a reference to the controller on the upload page in your form configuration, by adding
"controller": "UploadPageController"
.Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce
the testing if necessary.
Checklist: