Rename some address input event handlers
Categories
(Thunderbird :: Message Compose Window, task)
Tracking
(Not tracked)
People
(Reporter: thomas8, Assigned: thomas8)
References
Details
Attachments
(1 file, 1 obsolete file)
10.02 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
Coming from bug 1629364, it was a bit strange to add address input sanitation to a blur event handler function called resetAddressContainer(element)
, which now contains the following:
- 1 line to reset focus styling on the container (as function name suggests)
- 26 lines doing input blur stuff like creating pills and input whitespace sanitation (not matching the function name)
Note that the addressContainer is not the element on which the blur event actually occured, so the function name doesn't refer to the target of the event which it handles in any way.
So I figured we could rename some of these address input event handlers to make their actual purpose more obvious, using a common pattern.
Current Name | Proposed Name (generic pattern) |
---|---|
resetAddressContainer() | --> addressInputOnBlur() |
highlightAddressContainer() | --> addressInputOnFocus() |
recipientOnBeforeKeyDown() | --> addressInputOnBeforeKeyDown() |
Benefits of proposed naming pattern:
- more generic
- function name will still be appropriate even if we change/add content
- easier to understand/identify in code
- more informative/truthful about their purpose (because of including "addressInput" element reference and event name which delimits the context/use)
- less prone to hijacking for alien purposes due to misleading function name
Function comments updated and polished accordingly.
Assignee | ||
Comment 1•4 years ago
|
||
This renames the address input event handling functions as explained in comment 0.
Comment 2•4 years ago
|
||
Comment on attachment 9145015 [details] [diff] [review] 1634687_renameAddressInputEventHandlers.diff Review of attachment 9145015 [details] [diff] [review]: ----------------------------------------------------------------- For these architectural and code styling decisions is better to ping Magnus.
Comment 3•4 years ago
|
||
Comment on attachment 9145015 [details] [diff] [review] 1634687_renameAddressInputEventHandlers.diff Review of attachment 9145015 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +2217,3 @@ > }); > input.onBeforeHandleKeyDown = event => { > + addressInputOnBeforeKeyDown(event, input); For this one, wouldn't it be more appropriate to name it addressInputOnBeforeHandleKeyDown? But why are we passing in the input? I think you could always use event.target and drop the element parameter
Assignee | ||
Comment 4•4 years ago
|
||
updated patch per review of comment 3
Comment 5•4 years ago
|
||
Comment on attachment 9145842 [details] [diff] [review] 1634687_renameAddressInputEventHandlers.diff Review of attachment 9145842 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, r=mkmelin
Updated•4 years ago
|
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/74fa74c3ca0a
Rename some address input event handlers. r=mkmelin
Assignee | ||
Comment 7•4 years ago
|
||
Magnus, as you have approved these changes, is it ok with you if we expect incoming code to use this event handler naming pattern consistently? For all the reasons of comment 0, consistency, and a clear separation of layout and code with predictable, generic hooks.
Note that best practices (e.g. React) are recommending similar naming pattern:
https://jaketrent.com/post/naming-event-handlers-react/
https://medium.com/javascript-in-plain-english/handy-naming-conventions-for-event-handler-functions-props-in-react-fc1cbb791364
<tag id="someid"
onclick="someidOnClick(event);"
onkeydown="someidOnKeyDown(event);"/>
function someidOnClick(event) {
helperfunction(...) [if needed...]
}
function someidOnKeyDown(event) {
checks and ifs and buts...
do some stuff
helperfunction(...) [if needed...]
}
helperfunction(...) [called from elsewhere]
(In reply to Thomas D. from comment #0)
Current Name Proposed Name (generic pattern) resetAddressContainer() --> addressInputOnBlur() highlightAddressContainer() --> addressInputOnFocus() recipientOnBeforeKeyDown() --> addressInputOnBeforeKeyDown() Benefits of proposed naming pattern:
- more generic
- function name will still be appropriate even if we change/add content
- easier to understand/identify in code
- more informative/truthful about their purpose (because of including "addressInput" element reference and event name which delimits the context/use)
- less prone to hijacking for alien purposes due to misleading function name
Assignee | ||
Comment 9•4 years ago
•
|
||
(In reply to Magnus Melin [:mkmelin] from comment #8)
Sure.
Thanks!
Another coding style question:
If we have a choice between...
- creating dozens of hard-coded, complex event handling callers in code, to pass a specific associated element of each event target into a shared function called by event handlers (click and keydown), whereby keydown does not need this argument in 90% of cases; VS
- keeping all callers simple and retrieve the associated element inside the shared function, from a dedicated attribute of each event target, where the invariable layout association between target element and associated element is expressed in the layout;
... which one is preferable?
This is similar to the case of passing this
argument with callers, and you said it's better to retrieve the element from within the event handler function via event.target
and keep callers simple (and I have seen that you are right...).
Complex callers with extra associatedElement argument passed around all over
function onload(elementObject) {
targetElement.addEventListener("click", event => {
let associatedElementID = elementObject.associatedElementID;
targetOnClick(event, elementObject.associatedElementID);
});
targetElement.addEventListener("keydown", event => {
let associatedElementID = elementObject.associatedElementID;
targetOnKeydown(event, elementObject.associatedElementID);
});
}
function targetOnClick(event, associatedElementID) {
sharedFunction(event, associatedElementID)
}
function targetOnKeyDown(event, associatedElementID) {
if (event.key == "XYZ") {
// 90% of cases: associatedElementID not needed
}
if (event.key=="Enter") {
// 10% of cases:
sharedFunction(event, associatedElementID)
}
}
sharedFunction(event, associatedElementID) {
>> act on event target and associated element
}
VS
Simple callers, then retrieve associatedElement from layout, inside the final shared function where it's needed
function onload(elementObject) {
targetElement.addEventListener("click", event => {
targetOnClick(event);
});
targetElement.addEventListener("keydown", event => {
targetOnKeydown(event);
});
}
function targetOnClick(event) {
sharedFunction(event)
}
function targetOnKeyDown(event) {
if (event.key == "XYZ") {
...
}
if (event.key=="Enter") {
sharedFunction(event)
}
}
sharedFunction(event) {
let associatedElementID = event.target.dataset.associatedElementID;
>> act on event.target and associated element
}
Comment 10•4 years ago
|
||
I think the second version is much preferable and easier to read.
Assignee | ||
Comment 11•4 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #10)
I think the second version is much preferable and easier to read.
Thank you! Agreed! :-)
Assignee | ||
Updated•4 years ago
|
Description
•