JS Cleanup #198

nienna73 · 2021-05-17T20:51:18Z

What's in this PR:

removed all onclick attributes on buttons and links
added event listeners in place of all onclick attributes
moved all JS to its own file

What still needs doing:

tests! Lots of the functionality that relies on JS isn't covered in the tests

Notes:
This branch was originally created to solve this issue, where a single quotation mark in a phrase would prevent a user from editing it:

single-quote-issue.mov

eddieantonio

Looks good! I have tonnes of suggestions for how to embetter the JS, but that's better done in a pairing sesh than endless GH comments, haha. Feel free to reach out if you have any questions :)

EDIT: I do not understand the sprinkling of async on all of the event handlers when none of them use the await keyword or need to return a Promise. Is there a reason for these to be async?

eddieantonio · 2021-05-17T20:56:46Z

validation/jinja2/validation/_wrong_buttons.html

    <div class="menu__none rec-wrong-word" data-rec-id={{ recording.id }}>
        <form id="{{ recording.id }}-wrong-word" method="POST" action="/api/save_wrong_word/{{ recording.id }}">
            <input type="hidden" name="csrfmiddlewaretoken" value="{{ csrf_token }}">
            <input type="hidden" name="referrer" value="{{ request.build_absolute_uri() }}">
            What word is being spoken?
            <input type="text" id="wrong_word" name="wrong_word">
            <input form="{{ recording.id }}-wrong-word" type="submit" value="Save" class="button button--success button--small save-wrong-word" data-rec-id={{ recording.id }}>
-            <input type="button" class="button button--fail button--small" value="Cancel" data-rec-id={{ recording.id }})" onclick="hideWrongWordDiv('{{ recording.id }}')">
+            <input type="button" class="button button--fail button--small" value="Cancel" data-cy="cancel-wrong-word-button" data-rec-id={{ recording.id }})">


Should this be a <button>?

Suggested change

<input type="button" class="button button--fail button--small" value="Cancel" data-cy="cancel-wrong-word-button" data-rec-id={{ recording.id }})">

<button type="button" class="button button--fail button--small" data-cy="cancel-wrong-word-button" data-rec-id={{ recording.id }})">Cancel</button>

eddieantonio · 2021-05-17T21:06:38Z

validation/jinja2/validation/segment_details.html

@@ -45,11 +6,13 @@
 <div class="table-responsive">

    <div>
-        <button class="button button--success button--back" onclick='goBack()'>Back</button>
+        <button data-cy="back-button" class="button button--success button--back">Back</button>


This is a really pendatic note, but this should be an <a> with the back page as its href. If you know what page it should go "back" to in advance, that href can be hardcoded. Otherwise, this answer from StackOverflow is really nice:

// adapted from: https://stackoverflow.com/a/46163215/6626414 for (let el of document.querySelectorAll('a.button--back')) { el.setAttribute('href', document.referrer); // this remains the same! el.addEventListener('click', (evt) => { window.history.back(); }) }

eddieantonio · 2021-05-17T21:14:49Z

validation/jinja2/validation/segment_edit.html

@@ -4,6 +4,6 @@ <h5>Edit Segment</h5>
    <input type="hidden" name="csrfmiddlewaretoken" value="{{ csrf_token }}">
    {{ form }}
    <input data-cy="save-button" type="submit" class="button button--success" value="Save">
-    <input data-cy="cancel-button" type="button" class="button button--fail" onclick="hideDiv()" value="Cancel">
+    <input data-cy="cancel-button" type="button" class="button button--fail" value="Cancel">


Should this be a <button>?

Suggested change

<input data-cy="cancel-button" type="button" class="button button--fail" value="Cancel">

<button type="button" data-cy="cancel-button" type="button" class="button button--fail">Cancel</button>

eddieantonio · 2021-05-17T21:17:11Z

validation/static/editSegment.js

+
+document.addEventListener('DOMContentLoaded', () => {
+    for (let button of document.querySelectorAll('[data-segment-name]')) {
+        button.addEventListener("click", async (e) => {


Suggested change

button.addEventListener("click", async (e) => {

button.addEventListener("click", (e) => {

eddieantonio · 2021-05-17T21:17:42Z

validation/static/editSegment.js

+    }
+
+    for (let button of document.querySelectorAll('[data-cy="accept-button"], [data-cy="revert-button"]')) {
+        button.addEventListener("click", async(e) => {


Suggested change

button.addEventListener("click", async(e) => {

button.addEventListener("click", (e) => {

eddieantonio · 2021-05-17T21:19:00Z

validation/static/editSegment.js

+    }
+
+    for (let button of document.querySelectorAll('[data-cy="back-button"]')) {
+        button.addEventListener("click", async(e) => {


Suggested change

button.addEventListener("click", async(e) => {

button.addEventListener("click", (e) => {

eddieantonio · 2021-05-17T21:27:04Z

validation/static/editSegment.js

+
+function showDiv() {
+    document.getElementById('edit').style.display = "block";
+    document.getElementById('id_cree').focus()


Note: assigning focus programmatically is considered not a great practice, but here, I don't think it matters much. Always be careful with focus stealing.

eddieantonio · 2021-05-17T21:32:06Z

validation/static/menu.js

-        menu.classList.add("menu__none");
-        menu.classList.remove("menu__block");
+document.addEventListener("DOMContentLoaded", () => {
+    for (let link of document.querySelectorAll('[data-cy="show-flex-button"]')) {


So "flex" button refefers to 'flex as in "cirum" and not as in "-box". Because I thought this was a like a button { display: flex; } at first 😂 😂 😂

eddieantonio · 2021-05-17T21:32:30Z

validation/static/toggleWrongWordDiv.js

+
+document.addEventListener("DOMContentLoaded", () => {
+    for (let button of document.querySelectorAll('[data-cy="wrong-word-button"]')) {
+        button.addEventListener("click", async (e) => {


Suggested change

button.addEventListener("click", async (e) => {

button.addEventListener("click", (e) => {

eddieantonio · 2021-05-17T21:32:40Z

validation/static/toggleWrongWordDiv.js

+    }
+
+    for (let button of document.querySelectorAll('[data-cy="cancel-wrong-word-button"]')) {
+        button.addEventListener("click", async (e) => {


Suggested change

button.addEventListener("click", async (e) => {

button.addEventListener("click", (e) => {

andrewdotn

Looks good

Only general comment is that sometimes you’re putting semicolons at the ends of lines and sometimes not, so you might want to start using prettier to just do all the code formatting for you.

andrewdotn · 2021-05-18T15:16:56Z

validation/static/editSegment.js

+            const transcription = e.target.dataset.transcription
+            const translation = e.target.dataset.translation
+            const analysis = e.target.dataset.analysis


If you want to use some fancier syntax*

Suggested change

const transcription = e.target.dataset.transcription

const translation = e.target.dataset.translation

const analysis = e.target.dataset.analysis

const { transcription, translation, analysis } = e.target.dataset;

*I think this has now has excellent browser support

andrewdotn · 2021-05-18T15:20:37Z

validation/static/helpers.js

+// Helper functions that are needed across multiple JS files
+
+
+function getElementByPhraseId(className, phraseId) {


You should be able to do a lookup using the DOM API instead of having to do a linear scan:

document.querySelector(`.${className}[data-phrase-id=${phraseId}]`)

That said, I know there were issues with quoting related to the format of the ID that may make that trickier. That’s one reason that automatic numeric ID columns are so handy.

nienna73 · 2021-06-09T17:10:54Z

I converted this PR to a draft as it needs quite a bit of work and is no longer my top priority.

nienna73 added 10 commits May 14, 2021 15:02

fix: swapped single- and double-quotes

aa023c4

remove: accidental newline

936188a

wip: changed all onclicks to event listeners

d29ac36

wip: moved edit segment js to its own file

fa048d1

wip: refactor show div code into function

b07de69

wip: added event listeners to wrong word and speaker buttons

1ddeff4

wip: added event listener to circumflex button

19c7cf9

wip: commented helpers.js

29acddd

remove: unused function

5a02438

tests: updated cypress tests

8b55e90

eddieantonio approved these changes May 17, 2021

View reviewed changes

andrewdotn approved these changes May 18, 2021

View reviewed changes

nienna73 marked this pull request as draft June 9, 2021 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS Cleanup #198

JS Cleanup #198

nienna73 commented May 17, 2021 •

edited

Loading

eddieantonio left a comment •

edited

Loading

eddieantonio May 17, 2021

eddieantonio May 17, 2021

eddieantonio May 17, 2021

eddieantonio May 17, 2021

eddieantonio May 17, 2021

eddieantonio May 17, 2021

eddieantonio May 17, 2021

eddieantonio May 17, 2021

eddieantonio May 17, 2021

eddieantonio May 17, 2021

andrewdotn left a comment

andrewdotn May 18, 2021 •

edited

Loading

andrewdotn May 18, 2021

nienna73 commented Jun 9, 2021

	<input type="button" class="button button--fail button--small" value="Cancel" data-cy="cancel-wrong-word-button" data-rec-id={{ recording.id }})">
	<button type="button" class="button button--fail button--small" data-cy="cancel-wrong-word-button" data-rec-id={{ recording.id }})">Cancel</button>

	<input data-cy="cancel-button" type="button" class="button button--fail" value="Cancel">
	<button type="button" data-cy="cancel-button" type="button" class="button button--fail">Cancel</button>

	button.addEventListener("click", async (e) => {
	button.addEventListener("click", (e) => {

	button.addEventListener("click", async(e) => {
	button.addEventListener("click", (e) => {

		// Helper functions that are needed across multiple JS files


		function getElementByPhraseId(className, phraseId) {

JS Cleanup #198

Are you sure you want to change the base?

JS Cleanup #198

Conversation

nienna73 commented May 17, 2021 • edited Loading

What's in this PR:

What still needs doing:

eddieantonio left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewdotn left a comment

Choose a reason for hiding this comment

andrewdotn May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nienna73 commented Jun 9, 2021

nienna73 commented May 17, 2021 •

edited

Loading

eddieantonio left a comment •

edited

Loading

andrewdotn May 18, 2021 •

edited

Loading