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

Added ability to customize html-static-task-example submit button #879

Merged
merged 8 commits into from
Aug 3, 2022

Conversation

Etesam913
Copy link
Contributor

@Etesam913 Etesam913 commented Aug 1, 2022

Overview

#692 mentions giving the user the ability to disable the submit button in the html-static-task-example

This is now possible by writing the following in the demo_task.html

<script>
  window._MEPHISTO_CONFIG_.set("HIDE_SUBMIT_BUTTON", true)
</script>

The set(property, value) method sets the property to the value in the window._MEPHISTO_CONFIG object and also uses the newly created event emitter to emit an event of name property and argument value.

This is the event emitter(non-transpiled and non-minified version) that is defined in the wrap_crowd_source.js files

let eventEmitter = () => ({
  events: {},
  emit(event, ...args) {
    let callbacks = this.events[event] || [];
    for (let i = 0, length = callbacks.length; i < length; i++) {
      callbacks[i](...args);
    }
  },
  on(event, cb) {
    this.events[event]?.push(cb) || (this.events[event] = [cb]);
    return () => {
      this.events[event] = this.events[event]?.filter((i) => cb !== i);
    };
  },
});

Events can be listened to and make a state update using the useMephistoGlobalConfig hook.

For example, in the simple_static_task's app.jsx:

  const [
    isSubmitButtonHidden,
    setIsSubmitButtonHidden,
  ] = useMephistoGlobalConfig(
    "HIDE_SUBMIT_BUTTON",
    false,
    (val) => typeof val === "boolean"
  );

This code snippet updates the isSubmitButtonHidden state to whatever HIDE_SUBMIT_BUTTON was set to in demo_task.html.

The second param(optional) value, false, is the default value of isSubmitButtonHidden. This could be useful when the event is emitted after some action in the future. In this case it is emitted instantly at the beginning so it is not that useful.

The last param(optional) value, is a function that has to return true for the state to be updated. In this case, the function ensures that HIDE_SUBMIT_BUTTON was set to a boolean in demo_task.html before updating the state.

Video

Resolves #692

customize_submit_button_video.mov

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 1, 2022
@Etesam913 Etesam913 changed the title Added ability to customize html-static-task-example submit button Added ability to customizehtml-static-task-example submit button Aug 1, 2022
@Etesam913 Etesam913 changed the title Added ability to customizehtml-static-task-example submit button Added ability to customize html-static-task-example submit button Aug 1, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2022

Codecov Report

Merging #879 (0d5086c) into main (a5b1f78) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #879   +/-   ##
=======================================
  Coverage   64.61%   64.61%           
=======================================
  Files         108      108           
  Lines        9313     9313           
=======================================
  Hits         6018     6018           
  Misses       3295     3295           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@pringshia pringshia left a comment

Choose a reason for hiding this comment

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

I'm not sure this solution actually works. I explained why here.

What I imagine would work instead:

  1. We expose setter and getter methods for modifying the global _MEPHISTO_CONFIG_ object, e.g. _MEPHISTO_CONFIG_.get(x) and _MEPHISTO_CONFIG_.set(x, y)
  2. We expose an event emitter type object, that we can subscribe to events. _MEPHISTO_CONFIG_.subscribe(handler) and _MEPHISTO_CONFIG_.unsubscribe(handle) where handler looks something like (propName, val) => {...}
  3. The base blueprint React component subscribes to the handler to update the disabled state:
const [isHidden, setIsHidden] = React.useState(false);

const handler = React.useCallback((propName, val) => {
  if (propName === "HIDE_SUBMIT_BUTTON")
     setIsHidden(val);
}, [setIsHidden]);

React.useEffect(() => {
  window._MEPHISTO_CONFIG_.subscribe(handler)
  return () => window._MEPHISTO_CONFIG_.unsubscribe(handler);
}, [handler])

// later...

 { isHidden
    ? null
    : <Button
        type="submit"
        disabled={disabled}
      />
  }

@Etesam913
Copy link
Contributor Author

That definitely seems like a better solution, I forgot that there was a app.jsx file for some reason

Comment on lines 115 to 117
React.useEffect(() => {
window._MEPHISTO_CONFIG_.EVENT_EMITTER.on("HIDE_SUBMIT_BUTTON", handler);
}, [setIsHidden]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Runs the hideSubmitButton handler method when the HIDE_SUBMIT_BUTTON event is emitted.

Comment on lines 18 to 33
let eventEmitter = () => ({
events: {},
emit(event, ...args) {
let callbacks = this.events[event] || [];
for (let i = 0, length = callbacks.length; i < length; i++) {
callbacks[i](...args);
}
},
on(event, cb) {
this.events[event]?.push(cb) || (this.events[event] = [cb]);
return () => {
this.events[event] = this.events[event]?.filter((i) => cb !== i);
};
},
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The small event emitter class

Comment on lines +75 to +84
window._MEPHISTO_CONFIG_.get = (property) => {
if (!(property in window._MEPHISTO_CONFIG_))
throw new Error(`${property} does not exist in window.MEPHISTO_CONFIG`);
else return window._MEPHISTO_CONFIG_[property];
};

window._MEPHISTO_CONFIG_.set = (property, value) => {
window._MEPHISTO_CONFIG_[property] = value;
events.emit(property, value);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

getter and setter methods for the window.MEPHISTO_CONFIG object

Copy link
Contributor

@pringshia pringshia Aug 3, 2022

Choose a reason for hiding this comment

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

Hmm, I'm really torn / undecided if throwing an Error here at runtime is desirable or not. The downside is having tasks crash when launched on mTurk, the upside is that we flag logical errors quickly and loudly - developers should be able to detect these crashes immediately usually after an initial launch of their task.

I think ultimately this might be fine... still undecided though

}
},
on(event, cb) {
this.events[event]?.push(cb) || (this.events[event] = [cb]);
Copy link
Contributor

@pringshia pringshia Aug 3, 2022

Choose a reason for hiding this comment

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

just realized something. this file gets loaded in directly into the HTML page that runs on the client's browser:

<script src="wrap_crowd_source.js"></script>

As such, we maaaay run into some slight compat issues with "newer" ES6 syntax such as the spread syntax (...args) and optional chaining (?.) Seems like we lose support for ~5% of global users, or 1 out of 20 folks:

I would suggest using the Babel transpiler to make sure we use a target with more coverage. I suggest playing around with the targets value in the sidebar here:

CleanShot 2022-08-03 at 10 35 48@2x

Copy link
Contributor

@pringshia pringshia Aug 3, 2022

Choose a reason for hiding this comment

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

Let's also then use a JS minifer to compress the transpiled output above into maybe a single line of compressed output and then add that to the file to save some real estate. We can then leave a comment above it explaining our process for generating it nanoevents lib -> transpiler -> minifer

// The following is the nanoevents npm library (link) manually processed as such:
// 1. transpiled to support more browser targets using the Babel transpiler (link), and
// 2. minified using a JS minifier (link) 
var eventEmitter=function(){return{events:{},emit:function(f){for(var b=this.events[f]||[],c=arguments.length,e=new Array(c>1?c-1:0),a=1;a<c;a++)e[a-1]=arguments[a];for(var d=0,g=b.length;d<g;d++)b[d].apply(b,e)},on:function(b,c){var a,d=this;return(null===(a=this.events[b])|| void 0===a?void 0:a.push(c))||(this.events[b]=[c]),function(){var a;d.events[b]=null===(a=d.events[b])|| void 0===a?void 0:a.filter(function(a){return c!==a})}}}}

"HIDE_SUBMIT_BUTTON",
handleHideSubmitButtonEvent
);
}, [setIsSubmitButtonHidden]);
Copy link
Contributor

@pringshia pringshia Aug 3, 2022

Choose a reason for hiding this comment

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

Stretch goal:

I'm thinking all of this code could be condensed down into an elegant hook, very similar to the React.useState hook:

const [isSubmitButtonHidden, setIsSubmitButtonHidden]  = useMephistoGlobalConfig("HIDE_SUBMIT_BUTTON")

// with default value:
const [isSubmitButtonHidden, setIsSubmitButtonHidden]  = useMephistoGlobalConfig("HIDE_SUBMIT_BUTTON", false)

// optional, with validator function that only updates the first var under the hood if it passes the validation condition
const [isSubmitButtonHidden, setIsSubmitButtonHidden]  = useMephistoGlobalConfig("HIDE_SUBMIT_BUTTON", false, val => typeof val === "boolean")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I presume you mean the React.useState hook

Copy link
Contributor

Choose a reason for hiding this comment

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

oops yes, updated


/**
* This hook is to be used with events that are supposed to update state when they are consumed.
* @param {string} eventName
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit: I would rename eventName to configName.

I consider HIDE_SUBMIT_BUTTON to be a config property, not an event

* @param {function} validatorFunction
* @returns [any any]
*/
export function useMephistoGlobalConfig(
Copy link
Contributor

@pringshia pringshia Aug 3, 2022

Choose a reason for hiding this comment

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

this method looks great implementation-wise! Thanks for the jsdocs as well :)


React.useEffect(() => {
// Reset submitting when switching from onboarding
setSubmitting(false);
}, [currentTask]);
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

what was the rationale behind removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a mistake, I don't even know how that got updated.

@@ -15,6 +15,11 @@ Returning None for the assignment_id means that the task is being
previewed by the given worker.
\------------------------------------------*/

// The following is the nanoevents npm library (link) manually processed as such:
// 1. transpiled to support more browser targets using the Babel transpiler (link), and
// 2. minified using a JS minifier (link)
Copy link
Contributor

@pringshia pringshia Aug 3, 2022

Choose a reason for hiding this comment

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

(link) from my comment meant to actually replace that with the link (you can grab the links from my comment)

The babel one you might want to clean up a bit and remove the params otherwise it's really long and also includes the source code in the url

@@ -15,6 +15,11 @@ Returning None for the assignment_id means that the task is being
previewed by the given worker.
\------------------------------------------*/

// The following is the nanoevents npm library (link) manually processed as such:
// 1. transpiled to support more browser targets using the Babel transpiler (link), and
// 2. minified using a JS minifier (link)
Copy link
Contributor

Choose a reason for hiding this comment

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

replace link

if (validatorFunction(eventValue)) {
setConfigState(eventValue);
}
} else setConfigState(eventValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

both conditional paths are calling setConfigState here...?

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm, i see one is for the outer branch

Copy link
Contributor

@pringshia pringshia left a comment

Choose a reason for hiding this comment

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

Looking excellent! Thanks for the updated video in the PR description as well, that demoes this.

It would be good to add documentation for this feature here: https://github.com/facebookresearch/Mephisto/tree/main/mephisto/abstractions/blueprints/static_html_task

In the future it would be nice to add a README to the folder here: https://github.com/facebookresearch/Mephisto/tree/main/examples/simple_static_task

where we can also mention how the blueprint supports this functionality of hiding the button, and sample code to use this feature.

@Etesam913 Etesam913 merged commit 1470dd5 into main Aug 3, 2022
Sprint #15 - Affectionate Moose (Aug 15) automation moved this from In progress to Done Aug 3, 2022
@Etesam913 Etesam913 deleted the make-submit-button-customizable branch August 8, 2022 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Development

Successfully merging this pull request may close these issues.

Make the Submit button for the static_html_task blueprint customizable
4 participants