Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
the initialization is a big mess, there is an unrelated method in the Drupal.behavior.machineName object added.
Needs to be taken apart, and added to Drupal.machineName
for easier reuse and a more modular approach.
Comments
Comment #1
lorique CreditAttribution: lorique commentedI'm taking over this task. Will return once a review is in order.
Comment #2
lorique CreditAttribution: lorique commentedOK, refactoring done.
Now, when using machine-name.js you make a new Drupal.machineName(source_id, settings) which gives you an object to work with. There is a default _init function which implements the default behavior, and takes context as argument.
Its also possible to simply make use of some of the functionality in Drupal.machineName, such as the transliterate function, or eventDataHandler. I considered adding an example of how this was done, but i think thats more of a documentation thing than a coding thing. So perhaps we should add this to the d.o documentation for d8 instead.
Looking forward to some constructive reviews of my first ever patch to drupal :D
-- Lorique
Comment #3
lorique CreditAttribution: lorique commentedForgot to add "needs review"
Comment #4
nod_Yep, I like the separation of the different methods.
A couple of things, have a look at the tableheader.js file, there is the same kind of pattern we should be using here.
first the main constructor
Drupal.MachineName = function () {};
(it needs to have a capital letter first)Then we add a cache of all the created objects with
$.extend(Drupal.MachineName, {names : []});
Finally we add all the methods to the object:
Drupal.MachineName.prototype = {evenDataHandler: function () {}, clickEditHandler: function () {}};
And everything you have in the
_init
function can directly be in the constructorDrupal.MachineName
. so in the attach loop (which should be _after_ theit's not after in the tableheader file so we can keep it that way.) you'll do something likeDrupal.MachineName
Drupal.MachineName.names.push(new Drupal.MachineName(sourceId, settings, context));
Still, that's a good start, thanks!
Comment #5
lorique CreditAttribution: lorique commented@nod_
Alright, I should have implemented all the requested changes. I also changed source_id to sourceId just to keep things in line with the coding standard. I don't know why it was source_id before.
I tested locally, and it works as intended with the new changes, including caching the created objects.
Comment #6
lorique CreditAttribution: lorique commented- Added support for handling multiple errors using DrupalBehaviorError
- Implemented standardized extending behavior for Drupal.MachineName
- Renamed eventDataHandler to eventData because handlers should be events methods
- Made minor stylistic changes on request from @nod_
Comment #7
clemens.tolboomThere a a lot of whitespaces both on empty lines as at the end of sentences
Shouldn't we use
if (settings.machineName.hasOwnProperty(sourceID))
?Comment #8
lorique CreditAttribution: lorique commentedI found 1 line where it had whitespaces at the end of the line, and removed it. I also removed whitespaces on empty lines, these were added by my editor because it keeps my current identation level when i add a new line, and i didn't think they were important.
The if sentence you are referencing is in the constructor of the MachineName object.
Fixes are in the new patch.
Comment #9
clemens.tolboomJust one white-space left.
The : (colon) should not have a space before or is this a new code style?
(I'm new to javascript coding standards)
Comment #10
lorique CreditAttribution: lorique commentedWhite space removed.
You're right, there should be no white space in front of the colon, my bad. I just checked with the coding standards. I just do it for readability, bad habit i guess :)
Comment #11
aspilicious CreditAttribution: aspilicious commented+})(jQuery);
\ No newline at end of file
Comment #12
Kiphaas7 CreditAttribution: Kiphaas7 commented(readability) Could this be modified into a less longer line? Something like
Don't we want to use
.on()
here? And again, divide over multiple lines?Don't we want to use
.on()
here?(readability) Could this be modified into a less longer line? Something like
Comment #13
Jelle_Schanging status
Comment #14
lorique CreditAttribution: lorique commentedConverting to the .on() method seems like a good idea, but we should do it everywhere as a group then. Perhaps _nod can confirm or deny if we want to do this?
I don't see a point in doing it unless we do it for all core js, to provide it as a guideline to everyone creating contrib.
Comment #15
nod_We're in the process of changing everything to .on and .off, go for it. Also, we won't use .click and related shortcuts anymore. If you find some around change them as well please :)
Comment #16
lorique CreditAttribution: lorique commentedNew patch with some added readability as requested by Kiphaas7
Comment #18
Kiphaas7 CreditAttribution: Kiphaas7 commentedI glanced over it quickly, and it looks pretty darn nice. The error handling is also nice!
One minor gripe, though this might be personal preference:
Comment #19
Kiphaas7 CreditAttribution: Kiphaas7 commentedBONUS: Investigate if adding basic events and detach makes sense, as described in #1763812: [META] Provide complete attach/detach with basic events
Comment #20
lorique CreditAttribution: lorique commentedNew patch up. Bind has been replaced by .on, and at the request of Kip i have beautified the extend section where names cache are added.
Comment #22
lorique CreditAttribution: lorique commentedPatch failed testing because there was a change to machine-name.js from a different patch. I found it and brought the one line fix into my refactoring.
Comment #23
lorique CreditAttribution: lorique commentedComment #24
aspilicious CreditAttribution: aspilicious commented+})(jQuery);
\ No newline at end of file
Needs to end with a newline
Comment #25
lorique CreditAttribution: lorique commentedNew line added at end of file
Comment #26
lorique CreditAttribution: lorique commentedComment #27
nod_tag
Comment #28
seutje CreditAttribution: seutje commentedI'm confused as to why the hasOwnProperty check resides within the constructor, why not check for it before creating a new instance?
Comment #29
seutje CreditAttribution: seutje commentedwhy is this being called on the prototype and not on the instance?
Comment #30
martin107 CreditAttribution: martin107 commentedpatch no longer applies
Comment #31
m1r1k CreditAttribution: m1r1k commented@nod_ should we use this man for js testing? https://www.drupal.org/node/1780104
Comment #32
nod_for now we're going manual. The testswarm stuff is on the back burner.
Comment #33
Temoor CreditAttribution: Temoor commentedRerolled patch from #25
Comment #34
Temoor CreditAttribution: Temoor commentedComment #35
m1r1k CreditAttribution: m1r1k commentedComment #36
mikeker CreditAttribution: mikeker commentedNot sure if this is in scope for this issue or not... But I was having problems with the transliterate function flooding the server with requests when typing in the field that generates a machine name. If you're like me and end up correcting your spelling a lot, the machine name JS falls so far behind that I've ended up submitting the form with only part of the machine name entered correctly.
I added a cache and a delay so that the server is only pinged once ever 250ms. Bonus: we could make this delay configurable. Bigger bonus: we should cache based on the source and settings parameters in addition to the delay.
What do you think? Is this outside the scope of this issue?
Comment #37
mikeker CreditAttribution: mikeker commentedComment #38
mikeker CreditAttribution: mikeker commentedComment #39
nod_Thanks for the improvement but that's clearly a new feature. It's out of scope for this issue. Please open a new issue with the patch.
On the patch itself I have a couple of comments. We're adding yet more things to the behavior object (which is not encouraged) instead of exposing that in it's own object with explicit and documentable configuration. One nitpick, the conditions are backwards, please refer to the rest of the source code, we don't have yoda conditions anywhere else in the JS.
Clearly a needed improvement but out of scope for this :)
Comment #40
mikeker CreditAttribution: mikeker commented@_nod: Thanks for the call on in vs. out of scope. I figured as much... I've opened a new issue: #2353015: machine-name.js floods server with requests.
And thank you for the code review! Re: where to store values associated with this: This change would attach variables to the MachineName function at the global scope, not the behavior object. Or am I missing something? (Javascript is not my strong suit...)
re: Yoda conditionals: sorry about that -- old habit, though the code style guide is silent on the issue. Agreed, I couldn't find other examples in core so we shouldn't be introducing them here. Anyhow, I've fixed them in the patch on the issue referenced above.
Comment #41
mikeker CreditAttribution: mikeker commentedRemoving my uploads as that part has been moved elsewhere. Also resetting the status to where it was before I stuck my nose into this issue. :)
Comment #42
nod_Comment #43
Manjit.Singhrerolling a patch :)
Comment #44
googletorp CreditAttribution: googletorp as a volunteer commentedComment #45
nod_Reroll needs work, it doesn't apply properly, the resulting JS code is broken.
Comment #46
mikeker CreditAttribution: mikeker as a volunteer commentedAlso, the reroll should be against #33 as the changes I introduced in #36 were deemed out of scope. #43 is rerolling the wrong patch.
Comment #47
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedRerolled the patch on the basis of #33 comment.
As already mentioned by @mikeker, all the changes that he has introduced in #36has been deemed out of scope.
Comment #48
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedComment #49
Manjit.Singh@Nitesh Sethia Thanks for rerolling the patch. Please check my comments below.
Can you Please check this in your patch
Comment #50
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commented@Manjit didnt realise that I forgot to resolve the merge conflict.
Working on the patch.
Comment #51
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedComment #53
droplet CreditAttribution: droplet commentedNeeds JavaScript test case for big refactoring and new features
Comment #55
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI created a JavaScript test for the original machine-name.js and looked into the refactoring for this file.
As the original refactor is a bit outdated I decided to look into it again. The result is a bigger refactor than the first attempt.
The main difference is:
The rewrite still passes the created test but the test should ofcourse be reviewed separately.
This post only contains the test. The next comment will contain the test and rewrite.
Comment #56
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedTest + refactor.
Comment #58
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedNow in the correct format (hopefully).
Comment #59
droplet CreditAttribution: droplet commentedWelcome to JS World in Drupal.
I think you might be interested in this mission, take a look: #2809281: Use ES6 for core JavaScript development
Not a full review but few basic typo:
typo
$context, not defined yet?
Needs a better name I think
separated function for Transliteration
I expected one function call
Also, run ESLint :)
Comment #60
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedThanks for the quick feedback!
I'll start reading into the ES6 for core issue and run ESlint. After that I'll create another version that hopefully matches the new core standards.
Comment #61
LendudeDiscussed the test with @michielnugter offline, here are the improvements we came up with:
Please add a form to the form_test module to test this so you can lose the dependency on the user module
Use hardcoded expected values so any change in the output of the transliteration service is detected
super nitpick assertTrue is deprecated
Comment #62
droplet CreditAttribution: droplet commentedThanks for all great work. Could you both review this simple patch also. It also helps this refactoring.
#2668596: No delays update for Machine Name
Comment #63
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI updated the testcase to fix the mentioned issues. I didn't include an interdiff because the change is quite large.
Changes to the testcase:
- No more dependencies on the Transliteration service.
- Custom test form for the machine name so no dependency on the user add role form.
- No more deprecated testing methods
- Added testing for 2 machine name fields on a page
- Cleanup and coding standards issues fixed
I reviewed the other patch and will post my findings next, thank you for asking me to review and involving me in the process.
I have a question: as multiple issues now require a testcase for the machine name, is it maybe better to split the testcase into a separate issue and postpone this issue and the other issue(s) until the testcase is in?
As for the rewrite: I'm diving deep into ES6, Mocha and other changes happing to JavaScript in core, exiting stuff! I'll try to master the ES6 and Mocha a bit and then post a proposal for a ES6 version for the machine-name.js. Maybe the rewrite would also be a usefull testcase for implementing Mocha unit testing.
Comment #64
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #65
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #66
droplet CreditAttribution: droplet commentedYes, we can split the test to a new issue (in fact, it should be. this is a standard test, not just for this refactoring). and keep this active also. No conflicts :)
Comment #68
LendudeDon't think these changes to RoleForm were intended. They cause the test fails.
We should add a check before the click that the element is not visible before the click so we are actually detecting a change (Tried this and it fails, isVisible() is wonky)
$test_info is set in the foreach and now used outside it, not so nice.
Love the two machine name fields on the same page cause it shows the current code is broken for multiple machine name fields on a page:
Only one edit link!
And ++ on splitting off the test and opening a new issue for that and postponing the other issues/refactor on getting that in first.
Comment #69
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented1. Wow, whoops.. Glad to have tests :)
2. Completely right, a before check must be added. As to why the isVisible() doesn't work: the element is not actually hidden but positioned out of the way (to make sure any containing value is submitted to the server on submit). I changed the check to validate for the visually-hidden class.
3. Very true, I restructured the testinfo array so I don't need to be dirty anymore..
I'll post my new version with interdiff in a newly created issue.
Set to postponed untill the test is in.
Comment #70
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedSo after reading and learning a lot I found out ES6 is very nice indeed!
I'm not completely familiar with ES6 yet but I tried my best at a version of machine-name.js is ES6... Something to shoot at and it almost certainly not the thing you're looking for but hey, it might just be a start :)
Some explaination:
- I used classes and separated the machine name and transliteration into separate classes.
- I tried a modular approach although it currently doesn't work (I can't get Drupal to add the type="module" yet.
-- eslint is complaining about the import/export
- I tried to find a directory structure to place the classes in.
- The MachineName class is a minimal effort convert to ES6 and classes, much of the original code is still there. I thought it a little less relevant for now.
The first patch is how I intented it but that one doesn't work. The -working version actually works in browsers supporting ES6. I intentionally didn't use babel yet to convert them to see if the ES6 version actually works.
One important downsides to classes is that they're less easily partially overwritten. Views UI overwrites some drag-n-drop logic to add it's own stuff. This would be harder when using classes.
Very curious to hear what you (@droplet / @nod_ / anyone else with ES6 knowledge) think of this approach!
BTW: I am planning on adding a unit test using Mocha, that's the next step for me. I really want and would like to contribute to th ES6 effort.