JavaScript module to render and handle a form to add users Announcing the arrival of Valued...
Are my PIs rude or am I just being too sensitive?
How do I stop a creek from eroding my steep embankment?
Is there a documented rationale why the House Ways and Means chairman can demand tax info?
Gastric acid as a weapon
I am not a queen, who am I?
WAN encapsulation
Did Kevin spill real chili?
Does the Giant Rocktopus have a Swim Speed?
Models of set theory where not every set can be linearly ordered
Can inflation occur in a positive-sum game currency system such as the Stack Exchange reputation system?
Java 8 stream max() function argument type Comparator vs Comparable
How can I fade player when goes inside or outside of the area?
How to recreate this effect in Photoshop?
"Seemed to had" is it correct?
Is 1 ppb equal to 1 μg/kg?
Why was the term "discrete" used in discrete logarithm?
Is it possible to boil a liquid by just mixing many immiscible liquids together?
How discoverable are IPv6 addresses and AAAA names by potential attackers?
Check which numbers satisfy the condition [A*B*C = A! + B! + C!]
How to deal with a team lead who never gives me credit?
How widely used is the term Treppenwitz? Is it something that most Germans know?
How to draw this diagram using TikZ package?
Does accepting a pardon have any bearing on trying that person for the same crime in a sovereign jurisdiction?
What is a Meta algorithm?
JavaScript module to render and handle a form to add users
Announcing the arrival of Valued Associate #679: Cesar Manara
Planned maintenance scheduled April 17/18, 2019 at 00:00UTC (8:00pm US/Eastern)Showing statistics of popular JavaScript frameworks from GitHub APICode to call Space X API and display resultseDomForm - dynamic forms without writing any JavaScriptManaging a user databaseHow to automatically insert HTML to a form and obtain the inputs?Making an array editable by userRecognizing elements and doing event handling for repeating templatesUsing/storing server-side data on the client sideBest practice of HTML DOM template in javascriptJS micro-view libraryExpand form at runtime using a templateAuthentication system (no browser) with mail verification
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty{ margin-bottom:0;
}
$begingroup$
I recently switched to modular JavaScript and really like the idea of having the state of your application in JavaScript and not in the DOM. I want to know if what I am doing is considered best practices or not and how I can avoid re-rendering the entire list after each change.
First, the code:
HTML
<form id="add-user-form">
<input type="text" placeholder="username" name="username">
<button>add User</button>
</form>
<hr>
<ul id="user-list"></ul>
<script id="user-template" type="text/html">
<li data-key="${key}">${name}</li>
</script>
JavaScript
// lets you get templates from HTML ( as you can see in the HTML above ).
// This is to avoid writing out HTML inside JavaScript
const Template = {
cached: {},
get(templateId, placeholders) {
let templateHtml = ''
if (!this.cached[templateId]) {
templateHtml = document.getElementById(templateId).innerHTML.trim()
this.cached[templateId] = templateHtml
}
else {
templateHtml = this.cached[templateId]
}
for (key in placeholders) {
templateHtml = templateHtml.replace(new RegExp("\${\s*" + key + "\s*}", "g"), placeholders[key]);
}
return templateHtml
}
}
const Users = {
users: [],
init() {
this.cacheDom()
this.bindEvents()
},
cacheDom() {
this.addUserFormEl = document.getElementById('add-user-form')
this.userListEl = document.getElementById('user-list')
},
bindEvents() {
this.addUserFormEl.addEventListener('submit', this.handleAddUser.bind(this))
},
render() {
// gets the HTML from ALL users of our user list and replaces the current user list
const userHtml = this.users.map( user => Template.get('user-template', user) ).join('')
this.userListEl.innerHTML = userHtml
},
handleAddUser(e) {
e.preventDefault()
const username = e.currentTarget.elements['username'].value
if (!username) return
e.currentTarget.elements['username'].value = ''
const key = this.users.length // I know I know this is bad practice, please ignore :)
this.users.push({
key: key,
name: username
})
this.render()
}
}
Users.init()
The code has a form with an input field where you can type in a name that, once the form gets submitted, will be added to the user list below. It would probably be a good idea to separate the userlist and the userform into seperate concerns, but for the sake of this example I kept it simple.
I want to keep DOM modifications to a minimum so I put them inside the method Users.render. This however has one side-effect. Whenever I add an user, it reloads the entire list. It might work with this amount of data, but let's say I have thousands of records, maybe even with input fields. These would all be emptied again.
I could of course just add the new entry to the DOM inside the Users.handleAddUser method but what if later I also want to delete users? I would have to delete the entry within Users.handleRemoveUser and would then have two methods that handle DOM modifications for the same thing.
I know this all can be easily and beautifully achieved with frameworks such as VueJs that use a vDOM but I am looking for a vanillaJs way of handling this.
(Please note that the code is just an example. Yes Unit testing and typeScript are missing and I am well aware of the fact that the code will not run in browsers that do not support ES-6).
javascript html template ecmascript-6 dom
$endgroup$
add a comment |
$begingroup$
I recently switched to modular JavaScript and really like the idea of having the state of your application in JavaScript and not in the DOM. I want to know if what I am doing is considered best practices or not and how I can avoid re-rendering the entire list after each change.
First, the code:
HTML
<form id="add-user-form">
<input type="text" placeholder="username" name="username">
<button>add User</button>
</form>
<hr>
<ul id="user-list"></ul>
<script id="user-template" type="text/html">
<li data-key="${key}">${name}</li>
</script>
JavaScript
// lets you get templates from HTML ( as you can see in the HTML above ).
// This is to avoid writing out HTML inside JavaScript
const Template = {
cached: {},
get(templateId, placeholders) {
let templateHtml = ''
if (!this.cached[templateId]) {
templateHtml = document.getElementById(templateId).innerHTML.trim()
this.cached[templateId] = templateHtml
}
else {
templateHtml = this.cached[templateId]
}
for (key in placeholders) {
templateHtml = templateHtml.replace(new RegExp("\${\s*" + key + "\s*}", "g"), placeholders[key]);
}
return templateHtml
}
}
const Users = {
users: [],
init() {
this.cacheDom()
this.bindEvents()
},
cacheDom() {
this.addUserFormEl = document.getElementById('add-user-form')
this.userListEl = document.getElementById('user-list')
},
bindEvents() {
this.addUserFormEl.addEventListener('submit', this.handleAddUser.bind(this))
},
render() {
// gets the HTML from ALL users of our user list and replaces the current user list
const userHtml = this.users.map( user => Template.get('user-template', user) ).join('')
this.userListEl.innerHTML = userHtml
},
handleAddUser(e) {
e.preventDefault()
const username = e.currentTarget.elements['username'].value
if (!username) return
e.currentTarget.elements['username'].value = ''
const key = this.users.length // I know I know this is bad practice, please ignore :)
this.users.push({
key: key,
name: username
})
this.render()
}
}
Users.init()
The code has a form with an input field where you can type in a name that, once the form gets submitted, will be added to the user list below. It would probably be a good idea to separate the userlist and the userform into seperate concerns, but for the sake of this example I kept it simple.
I want to keep DOM modifications to a minimum so I put them inside the method Users.render. This however has one side-effect. Whenever I add an user, it reloads the entire list. It might work with this amount of data, but let's say I have thousands of records, maybe even with input fields. These would all be emptied again.
I could of course just add the new entry to the DOM inside the Users.handleAddUser method but what if later I also want to delete users? I would have to delete the entry within Users.handleRemoveUser and would then have two methods that handle DOM modifications for the same thing.
I know this all can be easily and beautifully achieved with frameworks such as VueJs that use a vDOM but I am looking for a vanillaJs way of handling this.
(Please note that the code is just an example. Yes Unit testing and typeScript are missing and I am well aware of the fact that the code will not run in browsers that do not support ES-6).
javascript html template ecmascript-6 dom
$endgroup$
add a comment |
$begingroup$
I recently switched to modular JavaScript and really like the idea of having the state of your application in JavaScript and not in the DOM. I want to know if what I am doing is considered best practices or not and how I can avoid re-rendering the entire list after each change.
First, the code:
HTML
<form id="add-user-form">
<input type="text" placeholder="username" name="username">
<button>add User</button>
</form>
<hr>
<ul id="user-list"></ul>
<script id="user-template" type="text/html">
<li data-key="${key}">${name}</li>
</script>
JavaScript
// lets you get templates from HTML ( as you can see in the HTML above ).
// This is to avoid writing out HTML inside JavaScript
const Template = {
cached: {},
get(templateId, placeholders) {
let templateHtml = ''
if (!this.cached[templateId]) {
templateHtml = document.getElementById(templateId).innerHTML.trim()
this.cached[templateId] = templateHtml
}
else {
templateHtml = this.cached[templateId]
}
for (key in placeholders) {
templateHtml = templateHtml.replace(new RegExp("\${\s*" + key + "\s*}", "g"), placeholders[key]);
}
return templateHtml
}
}
const Users = {
users: [],
init() {
this.cacheDom()
this.bindEvents()
},
cacheDom() {
this.addUserFormEl = document.getElementById('add-user-form')
this.userListEl = document.getElementById('user-list')
},
bindEvents() {
this.addUserFormEl.addEventListener('submit', this.handleAddUser.bind(this))
},
render() {
// gets the HTML from ALL users of our user list and replaces the current user list
const userHtml = this.users.map( user => Template.get('user-template', user) ).join('')
this.userListEl.innerHTML = userHtml
},
handleAddUser(e) {
e.preventDefault()
const username = e.currentTarget.elements['username'].value
if (!username) return
e.currentTarget.elements['username'].value = ''
const key = this.users.length // I know I know this is bad practice, please ignore :)
this.users.push({
key: key,
name: username
})
this.render()
}
}
Users.init()
The code has a form with an input field where you can type in a name that, once the form gets submitted, will be added to the user list below. It would probably be a good idea to separate the userlist and the userform into seperate concerns, but for the sake of this example I kept it simple.
I want to keep DOM modifications to a minimum so I put them inside the method Users.render. This however has one side-effect. Whenever I add an user, it reloads the entire list. It might work with this amount of data, but let's say I have thousands of records, maybe even with input fields. These would all be emptied again.
I could of course just add the new entry to the DOM inside the Users.handleAddUser method but what if later I also want to delete users? I would have to delete the entry within Users.handleRemoveUser and would then have two methods that handle DOM modifications for the same thing.
I know this all can be easily and beautifully achieved with frameworks such as VueJs that use a vDOM but I am looking for a vanillaJs way of handling this.
(Please note that the code is just an example. Yes Unit testing and typeScript are missing and I am well aware of the fact that the code will not run in browsers that do not support ES-6).
javascript html template ecmascript-6 dom
$endgroup$
I recently switched to modular JavaScript and really like the idea of having the state of your application in JavaScript and not in the DOM. I want to know if what I am doing is considered best practices or not and how I can avoid re-rendering the entire list after each change.
First, the code:
HTML
<form id="add-user-form">
<input type="text" placeholder="username" name="username">
<button>add User</button>
</form>
<hr>
<ul id="user-list"></ul>
<script id="user-template" type="text/html">
<li data-key="${key}">${name}</li>
</script>
JavaScript
// lets you get templates from HTML ( as you can see in the HTML above ).
// This is to avoid writing out HTML inside JavaScript
const Template = {
cached: {},
get(templateId, placeholders) {
let templateHtml = ''
if (!this.cached[templateId]) {
templateHtml = document.getElementById(templateId).innerHTML.trim()
this.cached[templateId] = templateHtml
}
else {
templateHtml = this.cached[templateId]
}
for (key in placeholders) {
templateHtml = templateHtml.replace(new RegExp("\${\s*" + key + "\s*}", "g"), placeholders[key]);
}
return templateHtml
}
}
const Users = {
users: [],
init() {
this.cacheDom()
this.bindEvents()
},
cacheDom() {
this.addUserFormEl = document.getElementById('add-user-form')
this.userListEl = document.getElementById('user-list')
},
bindEvents() {
this.addUserFormEl.addEventListener('submit', this.handleAddUser.bind(this))
},
render() {
// gets the HTML from ALL users of our user list and replaces the current user list
const userHtml = this.users.map( user => Template.get('user-template', user) ).join('')
this.userListEl.innerHTML = userHtml
},
handleAddUser(e) {
e.preventDefault()
const username = e.currentTarget.elements['username'].value
if (!username) return
e.currentTarget.elements['username'].value = ''
const key = this.users.length // I know I know this is bad practice, please ignore :)
this.users.push({
key: key,
name: username
})
this.render()
}
}
Users.init()
The code has a form with an input field where you can type in a name that, once the form gets submitted, will be added to the user list below. It would probably be a good idea to separate the userlist and the userform into seperate concerns, but for the sake of this example I kept it simple.
I want to keep DOM modifications to a minimum so I put them inside the method Users.render. This however has one side-effect. Whenever I add an user, it reloads the entire list. It might work with this amount of data, but let's say I have thousands of records, maybe even with input fields. These would all be emptied again.
I could of course just add the new entry to the DOM inside the Users.handleAddUser method but what if later I also want to delete users? I would have to delete the entry within Users.handleRemoveUser and would then have two methods that handle DOM modifications for the same thing.
I know this all can be easily and beautifully achieved with frameworks such as VueJs that use a vDOM but I am looking for a vanillaJs way of handling this.
(Please note that the code is just an example. Yes Unit testing and typeScript are missing and I am well aware of the fact that the code will not run in browsers that do not support ES-6).
javascript html template ecmascript-6 dom
javascript html template ecmascript-6 dom
edited Dec 5 '18 at 22:54
Sᴀᴍ Onᴇᴌᴀ
10.6k62168
10.6k62168
asked Mar 21 '18 at 2:53
MichaelMichael
212
212
add a comment |
add a comment |
1 Answer
1
active
oldest
votes
$begingroup$
Your Concerns/Questions
Whenever I add an user, it reloads the entire list.
One alternate approach would be to have the handleAddUser() store the newly added user in a property (e.g. recentlyAdded) and then have the render() method look for that property - if it is set (to something other than null) then add the rendered template of the new user and clear that property.
When the page loads, is there an existing list of users that gets added to the list? If so, maybe those could be stored in a different property and the existing code in render() could look for that property for rendering the existing records.
I could of course just add the new entry to the DOM inside the Users.handleAddUser method but what if later, I also want to delete users. I would have to delete the entry within
Users.handleRemoveUserand would so already have two methods that handle DOM modifications for the same thing.
Going along with the alternate approach above, one could have the handleRemoveUser method also store the key of the recently removed user, and then the render() method can look for that property too and remove such an element associated with that key - perhaps by adding an id or other data attribute to the template.
You didn't include an implementation for handleRemoveUser so I can only guess as to what it would be: some way of removing the user from the list this.user. If this.users is still an array, then looking for the user to remove might require a loop. However if this.users is an associative-array (i.e. object with keys corresponding to the key of each user) (or a Map), then looking for the user to remove can be achieved without a loop.
See this demonstrated in the snippet below.
In revision 2 you had this question, altered in revision 5:
As you can see I am not getting the input by ID but by form.elements and then referencing the name.
It is fine to do that, but because the form data doesn't get sent to the server-side, you could also use the id attribute on the input instead of the name attribute, and then fetch that element by id.
Other Feedback
The Template.get() function loops over the keys in placeholders:
for (key in placeholders) {
For this loop, key is a global variable, which may or not be intentional. In general it is best to avoid global variables. One reason would be that if code in a separate function makes its own variable with the same name, there would be no chance of overwriting the value if it isn't a global variable. Use const to declare key as local to that loop.
for (const key in placeholders) {
I recently saw this SO answer that offers a solution to interpolating a string literal similar to how a template literal would be interpolated. I compared it with the regular expression approach in your code and found it to be much slower (see this jsPerf test). I was hoping I could offer that suggestion but unfortunately it can be much slower.
// lets you get templates from HTML ( as you can see in the HTML above ).
// This is to avoid writing out HTML inside JavaScript
const Template = {
cached: {},
get(templateId, placeholders) {
let templateHtml = ''
if (!this.cached[templateId]) {
templateHtml = document.getElementById(templateId).innerHTML.trim()
this.cached[templateId] = templateHtml
}
else {
templateHtml = this.cached[templateId]
}
for (let key in placeholders) {
templateHtml = templateHtml.replace(new RegExp("\${\s*" + key + "\s*}", "g"), placeholders[key]);
}
return templateHtml
}
}
const Users = {
users: {},
recentlyAdded: null,
recentlyRemovedKey: null,
nextKey: 1,
init() {
this.cacheDom()
this.bindEvents()
},
cacheDom() {
this.addUserFormEl = document.getElementById('add-user-form')
this.userListEl = document.getElementById('user-list')
},
bindEvents() {
this.addUserFormEl.addEventListener('submit', this.handleAddUser.bind(this))
this.userListEl.addEventListener('click', this.handleListClick.bind(this));
},
render() {
// gets the HTML from ALL users of our user list and replaces the current user list
/*const userHtml = this.users.map( user => Template.get('user-template', user) ).join('')
this.userListEl.innerHTML = userHtml*/
if (this.recentlyAdded) {
this.userListEl.innerHTML += Template.get('user-template', this.recentlyAdded);
this.recentlyAdded = null;
}
if (this.recentlyRemovedKey) {
const element = document.getElementById(this.recentlyRemovedKey);
if (element) {
element.remove();
}
this.recentlyRemoved = null;
}
console.log('userlist after render: ',this.users);
},
handleAddUser(e) {
e.preventDefault()
const username = e.currentTarget.elements['username'].value
if (!username) return
e.currentTarget.elements['username'].value = ''
const key = this.nextKey++;
this.users[key] = {
key: key,
name: username
};
this.recentlyAdded = this.users[key];
this.render();
},
handleListClick(e) {
const target = e.target;
if (target.classList.contains('remove')) {
this.handleRemoveUser(target.parentNode.id);
}
},
handleRemoveUser(key) {
delete this.users[key];
this.recentlyRemovedKey = key;
this.render();
}
}
Users.init().remove:before {
content: 'X';
}<form id="add-user-form">
<input type="text" placeholder="username" name="username">
<button>add User</button>
</form>
<hr>
<ul id="user-list"></ul>
<script id="user-template" type="text/html">
<li data-key="${key}" id="${key}">${name} <button class="remove"></button></li>
</script>$endgroup$
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f190088%2fjavascript-module-to-render-and-handle-a-form-to-add-users%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
Your Concerns/Questions
Whenever I add an user, it reloads the entire list.
One alternate approach would be to have the handleAddUser() store the newly added user in a property (e.g. recentlyAdded) and then have the render() method look for that property - if it is set (to something other than null) then add the rendered template of the new user and clear that property.
When the page loads, is there an existing list of users that gets added to the list? If so, maybe those could be stored in a different property and the existing code in render() could look for that property for rendering the existing records.
I could of course just add the new entry to the DOM inside the Users.handleAddUser method but what if later, I also want to delete users. I would have to delete the entry within
Users.handleRemoveUserand would so already have two methods that handle DOM modifications for the same thing.
Going along with the alternate approach above, one could have the handleRemoveUser method also store the key of the recently removed user, and then the render() method can look for that property too and remove such an element associated with that key - perhaps by adding an id or other data attribute to the template.
You didn't include an implementation for handleRemoveUser so I can only guess as to what it would be: some way of removing the user from the list this.user. If this.users is still an array, then looking for the user to remove might require a loop. However if this.users is an associative-array (i.e. object with keys corresponding to the key of each user) (or a Map), then looking for the user to remove can be achieved without a loop.
See this demonstrated in the snippet below.
In revision 2 you had this question, altered in revision 5:
As you can see I am not getting the input by ID but by form.elements and then referencing the name.
It is fine to do that, but because the form data doesn't get sent to the server-side, you could also use the id attribute on the input instead of the name attribute, and then fetch that element by id.
Other Feedback
The Template.get() function loops over the keys in placeholders:
for (key in placeholders) {
For this loop, key is a global variable, which may or not be intentional. In general it is best to avoid global variables. One reason would be that if code in a separate function makes its own variable with the same name, there would be no chance of overwriting the value if it isn't a global variable. Use const to declare key as local to that loop.
for (const key in placeholders) {
I recently saw this SO answer that offers a solution to interpolating a string literal similar to how a template literal would be interpolated. I compared it with the regular expression approach in your code and found it to be much slower (see this jsPerf test). I was hoping I could offer that suggestion but unfortunately it can be much slower.
// lets you get templates from HTML ( as you can see in the HTML above ).
// This is to avoid writing out HTML inside JavaScript
const Template = {
cached: {},
get(templateId, placeholders) {
let templateHtml = ''
if (!this.cached[templateId]) {
templateHtml = document.getElementById(templateId).innerHTML.trim()
this.cached[templateId] = templateHtml
}
else {
templateHtml = this.cached[templateId]
}
for (let key in placeholders) {
templateHtml = templateHtml.replace(new RegExp("\${\s*" + key + "\s*}", "g"), placeholders[key]);
}
return templateHtml
}
}
const Users = {
users: {},
recentlyAdded: null,
recentlyRemovedKey: null,
nextKey: 1,
init() {
this.cacheDom()
this.bindEvents()
},
cacheDom() {
this.addUserFormEl = document.getElementById('add-user-form')
this.userListEl = document.getElementById('user-list')
},
bindEvents() {
this.addUserFormEl.addEventListener('submit', this.handleAddUser.bind(this))
this.userListEl.addEventListener('click', this.handleListClick.bind(this));
},
render() {
// gets the HTML from ALL users of our user list and replaces the current user list
/*const userHtml = this.users.map( user => Template.get('user-template', user) ).join('')
this.userListEl.innerHTML = userHtml*/
if (this.recentlyAdded) {
this.userListEl.innerHTML += Template.get('user-template', this.recentlyAdded);
this.recentlyAdded = null;
}
if (this.recentlyRemovedKey) {
const element = document.getElementById(this.recentlyRemovedKey);
if (element) {
element.remove();
}
this.recentlyRemoved = null;
}
console.log('userlist after render: ',this.users);
},
handleAddUser(e) {
e.preventDefault()
const username = e.currentTarget.elements['username'].value
if (!username) return
e.currentTarget.elements['username'].value = ''
const key = this.nextKey++;
this.users[key] = {
key: key,
name: username
};
this.recentlyAdded = this.users[key];
this.render();
},
handleListClick(e) {
const target = e.target;
if (target.classList.contains('remove')) {
this.handleRemoveUser(target.parentNode.id);
}
},
handleRemoveUser(key) {
delete this.users[key];
this.recentlyRemovedKey = key;
this.render();
}
}
Users.init().remove:before {
content: 'X';
}<form id="add-user-form">
<input type="text" placeholder="username" name="username">
<button>add User</button>
</form>
<hr>
<ul id="user-list"></ul>
<script id="user-template" type="text/html">
<li data-key="${key}" id="${key}">${name} <button class="remove"></button></li>
</script>$endgroup$
add a comment |
$begingroup$
Your Concerns/Questions
Whenever I add an user, it reloads the entire list.
One alternate approach would be to have the handleAddUser() store the newly added user in a property (e.g. recentlyAdded) and then have the render() method look for that property - if it is set (to something other than null) then add the rendered template of the new user and clear that property.
When the page loads, is there an existing list of users that gets added to the list? If so, maybe those could be stored in a different property and the existing code in render() could look for that property for rendering the existing records.
I could of course just add the new entry to the DOM inside the Users.handleAddUser method but what if later, I also want to delete users. I would have to delete the entry within
Users.handleRemoveUserand would so already have two methods that handle DOM modifications for the same thing.
Going along with the alternate approach above, one could have the handleRemoveUser method also store the key of the recently removed user, and then the render() method can look for that property too and remove such an element associated with that key - perhaps by adding an id or other data attribute to the template.
You didn't include an implementation for handleRemoveUser so I can only guess as to what it would be: some way of removing the user from the list this.user. If this.users is still an array, then looking for the user to remove might require a loop. However if this.users is an associative-array (i.e. object with keys corresponding to the key of each user) (or a Map), then looking for the user to remove can be achieved without a loop.
See this demonstrated in the snippet below.
In revision 2 you had this question, altered in revision 5:
As you can see I am not getting the input by ID but by form.elements and then referencing the name.
It is fine to do that, but because the form data doesn't get sent to the server-side, you could also use the id attribute on the input instead of the name attribute, and then fetch that element by id.
Other Feedback
The Template.get() function loops over the keys in placeholders:
for (key in placeholders) {
For this loop, key is a global variable, which may or not be intentional. In general it is best to avoid global variables. One reason would be that if code in a separate function makes its own variable with the same name, there would be no chance of overwriting the value if it isn't a global variable. Use const to declare key as local to that loop.
for (const key in placeholders) {
I recently saw this SO answer that offers a solution to interpolating a string literal similar to how a template literal would be interpolated. I compared it with the regular expression approach in your code and found it to be much slower (see this jsPerf test). I was hoping I could offer that suggestion but unfortunately it can be much slower.
// lets you get templates from HTML ( as you can see in the HTML above ).
// This is to avoid writing out HTML inside JavaScript
const Template = {
cached: {},
get(templateId, placeholders) {
let templateHtml = ''
if (!this.cached[templateId]) {
templateHtml = document.getElementById(templateId).innerHTML.trim()
this.cached[templateId] = templateHtml
}
else {
templateHtml = this.cached[templateId]
}
for (let key in placeholders) {
templateHtml = templateHtml.replace(new RegExp("\${\s*" + key + "\s*}", "g"), placeholders[key]);
}
return templateHtml
}
}
const Users = {
users: {},
recentlyAdded: null,
recentlyRemovedKey: null,
nextKey: 1,
init() {
this.cacheDom()
this.bindEvents()
},
cacheDom() {
this.addUserFormEl = document.getElementById('add-user-form')
this.userListEl = document.getElementById('user-list')
},
bindEvents() {
this.addUserFormEl.addEventListener('submit', this.handleAddUser.bind(this))
this.userListEl.addEventListener('click', this.handleListClick.bind(this));
},
render() {
// gets the HTML from ALL users of our user list and replaces the current user list
/*const userHtml = this.users.map( user => Template.get('user-template', user) ).join('')
this.userListEl.innerHTML = userHtml*/
if (this.recentlyAdded) {
this.userListEl.innerHTML += Template.get('user-template', this.recentlyAdded);
this.recentlyAdded = null;
}
if (this.recentlyRemovedKey) {
const element = document.getElementById(this.recentlyRemovedKey);
if (element) {
element.remove();
}
this.recentlyRemoved = null;
}
console.log('userlist after render: ',this.users);
},
handleAddUser(e) {
e.preventDefault()
const username = e.currentTarget.elements['username'].value
if (!username) return
e.currentTarget.elements['username'].value = ''
const key = this.nextKey++;
this.users[key] = {
key: key,
name: username
};
this.recentlyAdded = this.users[key];
this.render();
},
handleListClick(e) {
const target = e.target;
if (target.classList.contains('remove')) {
this.handleRemoveUser(target.parentNode.id);
}
},
handleRemoveUser(key) {
delete this.users[key];
this.recentlyRemovedKey = key;
this.render();
}
}
Users.init().remove:before {
content: 'X';
}<form id="add-user-form">
<input type="text" placeholder="username" name="username">
<button>add User</button>
</form>
<hr>
<ul id="user-list"></ul>
<script id="user-template" type="text/html">
<li data-key="${key}" id="${key}">${name} <button class="remove"></button></li>
</script>$endgroup$
add a comment |
$begingroup$
Your Concerns/Questions
Whenever I add an user, it reloads the entire list.
One alternate approach would be to have the handleAddUser() store the newly added user in a property (e.g. recentlyAdded) and then have the render() method look for that property - if it is set (to something other than null) then add the rendered template of the new user and clear that property.
When the page loads, is there an existing list of users that gets added to the list? If so, maybe those could be stored in a different property and the existing code in render() could look for that property for rendering the existing records.
I could of course just add the new entry to the DOM inside the Users.handleAddUser method but what if later, I also want to delete users. I would have to delete the entry within
Users.handleRemoveUserand would so already have two methods that handle DOM modifications for the same thing.
Going along with the alternate approach above, one could have the handleRemoveUser method also store the key of the recently removed user, and then the render() method can look for that property too and remove such an element associated with that key - perhaps by adding an id or other data attribute to the template.
You didn't include an implementation for handleRemoveUser so I can only guess as to what it would be: some way of removing the user from the list this.user. If this.users is still an array, then looking for the user to remove might require a loop. However if this.users is an associative-array (i.e. object with keys corresponding to the key of each user) (or a Map), then looking for the user to remove can be achieved without a loop.
See this demonstrated in the snippet below.
In revision 2 you had this question, altered in revision 5:
As you can see I am not getting the input by ID but by form.elements and then referencing the name.
It is fine to do that, but because the form data doesn't get sent to the server-side, you could also use the id attribute on the input instead of the name attribute, and then fetch that element by id.
Other Feedback
The Template.get() function loops over the keys in placeholders:
for (key in placeholders) {
For this loop, key is a global variable, which may or not be intentional. In general it is best to avoid global variables. One reason would be that if code in a separate function makes its own variable with the same name, there would be no chance of overwriting the value if it isn't a global variable. Use const to declare key as local to that loop.
for (const key in placeholders) {
I recently saw this SO answer that offers a solution to interpolating a string literal similar to how a template literal would be interpolated. I compared it with the regular expression approach in your code and found it to be much slower (see this jsPerf test). I was hoping I could offer that suggestion but unfortunately it can be much slower.
// lets you get templates from HTML ( as you can see in the HTML above ).
// This is to avoid writing out HTML inside JavaScript
const Template = {
cached: {},
get(templateId, placeholders) {
let templateHtml = ''
if (!this.cached[templateId]) {
templateHtml = document.getElementById(templateId).innerHTML.trim()
this.cached[templateId] = templateHtml
}
else {
templateHtml = this.cached[templateId]
}
for (let key in placeholders) {
templateHtml = templateHtml.replace(new RegExp("\${\s*" + key + "\s*}", "g"), placeholders[key]);
}
return templateHtml
}
}
const Users = {
users: {},
recentlyAdded: null,
recentlyRemovedKey: null,
nextKey: 1,
init() {
this.cacheDom()
this.bindEvents()
},
cacheDom() {
this.addUserFormEl = document.getElementById('add-user-form')
this.userListEl = document.getElementById('user-list')
},
bindEvents() {
this.addUserFormEl.addEventListener('submit', this.handleAddUser.bind(this))
this.userListEl.addEventListener('click', this.handleListClick.bind(this));
},
render() {
// gets the HTML from ALL users of our user list and replaces the current user list
/*const userHtml = this.users.map( user => Template.get('user-template', user) ).join('')
this.userListEl.innerHTML = userHtml*/
if (this.recentlyAdded) {
this.userListEl.innerHTML += Template.get('user-template', this.recentlyAdded);
this.recentlyAdded = null;
}
if (this.recentlyRemovedKey) {
const element = document.getElementById(this.recentlyRemovedKey);
if (element) {
element.remove();
}
this.recentlyRemoved = null;
}
console.log('userlist after render: ',this.users);
},
handleAddUser(e) {
e.preventDefault()
const username = e.currentTarget.elements['username'].value
if (!username) return
e.currentTarget.elements['username'].value = ''
const key = this.nextKey++;
this.users[key] = {
key: key,
name: username
};
this.recentlyAdded = this.users[key];
this.render();
},
handleListClick(e) {
const target = e.target;
if (target.classList.contains('remove')) {
this.handleRemoveUser(target.parentNode.id);
}
},
handleRemoveUser(key) {
delete this.users[key];
this.recentlyRemovedKey = key;
this.render();
}
}
Users.init().remove:before {
content: 'X';
}<form id="add-user-form">
<input type="text" placeholder="username" name="username">
<button>add User</button>
</form>
<hr>
<ul id="user-list"></ul>
<script id="user-template" type="text/html">
<li data-key="${key}" id="${key}">${name} <button class="remove"></button></li>
</script>$endgroup$
Your Concerns/Questions
Whenever I add an user, it reloads the entire list.
One alternate approach would be to have the handleAddUser() store the newly added user in a property (e.g. recentlyAdded) and then have the render() method look for that property - if it is set (to something other than null) then add the rendered template of the new user and clear that property.
When the page loads, is there an existing list of users that gets added to the list? If so, maybe those could be stored in a different property and the existing code in render() could look for that property for rendering the existing records.
I could of course just add the new entry to the DOM inside the Users.handleAddUser method but what if later, I also want to delete users. I would have to delete the entry within
Users.handleRemoveUserand would so already have two methods that handle DOM modifications for the same thing.
Going along with the alternate approach above, one could have the handleRemoveUser method also store the key of the recently removed user, and then the render() method can look for that property too and remove such an element associated with that key - perhaps by adding an id or other data attribute to the template.
You didn't include an implementation for handleRemoveUser so I can only guess as to what it would be: some way of removing the user from the list this.user. If this.users is still an array, then looking for the user to remove might require a loop. However if this.users is an associative-array (i.e. object with keys corresponding to the key of each user) (or a Map), then looking for the user to remove can be achieved without a loop.
See this demonstrated in the snippet below.
In revision 2 you had this question, altered in revision 5:
As you can see I am not getting the input by ID but by form.elements and then referencing the name.
It is fine to do that, but because the form data doesn't get sent to the server-side, you could also use the id attribute on the input instead of the name attribute, and then fetch that element by id.
Other Feedback
The Template.get() function loops over the keys in placeholders:
for (key in placeholders) {
For this loop, key is a global variable, which may or not be intentional. In general it is best to avoid global variables. One reason would be that if code in a separate function makes its own variable with the same name, there would be no chance of overwriting the value if it isn't a global variable. Use const to declare key as local to that loop.
for (const key in placeholders) {
I recently saw this SO answer that offers a solution to interpolating a string literal similar to how a template literal would be interpolated. I compared it with the regular expression approach in your code and found it to be much slower (see this jsPerf test). I was hoping I could offer that suggestion but unfortunately it can be much slower.
// lets you get templates from HTML ( as you can see in the HTML above ).
// This is to avoid writing out HTML inside JavaScript
const Template = {
cached: {},
get(templateId, placeholders) {
let templateHtml = ''
if (!this.cached[templateId]) {
templateHtml = document.getElementById(templateId).innerHTML.trim()
this.cached[templateId] = templateHtml
}
else {
templateHtml = this.cached[templateId]
}
for (let key in placeholders) {
templateHtml = templateHtml.replace(new RegExp("\${\s*" + key + "\s*}", "g"), placeholders[key]);
}
return templateHtml
}
}
const Users = {
users: {},
recentlyAdded: null,
recentlyRemovedKey: null,
nextKey: 1,
init() {
this.cacheDom()
this.bindEvents()
},
cacheDom() {
this.addUserFormEl = document.getElementById('add-user-form')
this.userListEl = document.getElementById('user-list')
},
bindEvents() {
this.addUserFormEl.addEventListener('submit', this.handleAddUser.bind(this))
this.userListEl.addEventListener('click', this.handleListClick.bind(this));
},
render() {
// gets the HTML from ALL users of our user list and replaces the current user list
/*const userHtml = this.users.map( user => Template.get('user-template', user) ).join('')
this.userListEl.innerHTML = userHtml*/
if (this.recentlyAdded) {
this.userListEl.innerHTML += Template.get('user-template', this.recentlyAdded);
this.recentlyAdded = null;
}
if (this.recentlyRemovedKey) {
const element = document.getElementById(this.recentlyRemovedKey);
if (element) {
element.remove();
}
this.recentlyRemoved = null;
}
console.log('userlist after render: ',this.users);
},
handleAddUser(e) {
e.preventDefault()
const username = e.currentTarget.elements['username'].value
if (!username) return
e.currentTarget.elements['username'].value = ''
const key = this.nextKey++;
this.users[key] = {
key: key,
name: username
};
this.recentlyAdded = this.users[key];
this.render();
},
handleListClick(e) {
const target = e.target;
if (target.classList.contains('remove')) {
this.handleRemoveUser(target.parentNode.id);
}
},
handleRemoveUser(key) {
delete this.users[key];
this.recentlyRemovedKey = key;
this.render();
}
}
Users.init().remove:before {
content: 'X';
}<form id="add-user-form">
<input type="text" placeholder="username" name="username">
<button>add User</button>
</form>
<hr>
<ul id="user-list"></ul>
<script id="user-template" type="text/html">
<li data-key="${key}" id="${key}">${name} <button class="remove"></button></li>
</script>// lets you get templates from HTML ( as you can see in the HTML above ).
// This is to avoid writing out HTML inside JavaScript
const Template = {
cached: {},
get(templateId, placeholders) {
let templateHtml = ''
if (!this.cached[templateId]) {
templateHtml = document.getElementById(templateId).innerHTML.trim()
this.cached[templateId] = templateHtml
}
else {
templateHtml = this.cached[templateId]
}
for (let key in placeholders) {
templateHtml = templateHtml.replace(new RegExp("\${\s*" + key + "\s*}", "g"), placeholders[key]);
}
return templateHtml
}
}
const Users = {
users: {},
recentlyAdded: null,
recentlyRemovedKey: null,
nextKey: 1,
init() {
this.cacheDom()
this.bindEvents()
},
cacheDom() {
this.addUserFormEl = document.getElementById('add-user-form')
this.userListEl = document.getElementById('user-list')
},
bindEvents() {
this.addUserFormEl.addEventListener('submit', this.handleAddUser.bind(this))
this.userListEl.addEventListener('click', this.handleListClick.bind(this));
},
render() {
// gets the HTML from ALL users of our user list and replaces the current user list
/*const userHtml = this.users.map( user => Template.get('user-template', user) ).join('')
this.userListEl.innerHTML = userHtml*/
if (this.recentlyAdded) {
this.userListEl.innerHTML += Template.get('user-template', this.recentlyAdded);
this.recentlyAdded = null;
}
if (this.recentlyRemovedKey) {
const element = document.getElementById(this.recentlyRemovedKey);
if (element) {
element.remove();
}
this.recentlyRemoved = null;
}
console.log('userlist after render: ',this.users);
},
handleAddUser(e) {
e.preventDefault()
const username = e.currentTarget.elements['username'].value
if (!username) return
e.currentTarget.elements['username'].value = ''
const key = this.nextKey++;
this.users[key] = {
key: key,
name: username
};
this.recentlyAdded = this.users[key];
this.render();
},
handleListClick(e) {
const target = e.target;
if (target.classList.contains('remove')) {
this.handleRemoveUser(target.parentNode.id);
}
},
handleRemoveUser(key) {
delete this.users[key];
this.recentlyRemovedKey = key;
this.render();
}
}
Users.init().remove:before {
content: 'X';
}<form id="add-user-form">
<input type="text" placeholder="username" name="username">
<button>add User</button>
</form>
<hr>
<ul id="user-list"></ul>
<script id="user-template" type="text/html">
<li data-key="${key}" id="${key}">${name} <button class="remove"></button></li>
</script>// lets you get templates from HTML ( as you can see in the HTML above ).
// This is to avoid writing out HTML inside JavaScript
const Template = {
cached: {},
get(templateId, placeholders) {
let templateHtml = ''
if (!this.cached[templateId]) {
templateHtml = document.getElementById(templateId).innerHTML.trim()
this.cached[templateId] = templateHtml
}
else {
templateHtml = this.cached[templateId]
}
for (let key in placeholders) {
templateHtml = templateHtml.replace(new RegExp("\${\s*" + key + "\s*}", "g"), placeholders[key]);
}
return templateHtml
}
}
const Users = {
users: {},
recentlyAdded: null,
recentlyRemovedKey: null,
nextKey: 1,
init() {
this.cacheDom()
this.bindEvents()
},
cacheDom() {
this.addUserFormEl = document.getElementById('add-user-form')
this.userListEl = document.getElementById('user-list')
},
bindEvents() {
this.addUserFormEl.addEventListener('submit', this.handleAddUser.bind(this))
this.userListEl.addEventListener('click', this.handleListClick.bind(this));
},
render() {
// gets the HTML from ALL users of our user list and replaces the current user list
/*const userHtml = this.users.map( user => Template.get('user-template', user) ).join('')
this.userListEl.innerHTML = userHtml*/
if (this.recentlyAdded) {
this.userListEl.innerHTML += Template.get('user-template', this.recentlyAdded);
this.recentlyAdded = null;
}
if (this.recentlyRemovedKey) {
const element = document.getElementById(this.recentlyRemovedKey);
if (element) {
element.remove();
}
this.recentlyRemoved = null;
}
console.log('userlist after render: ',this.users);
},
handleAddUser(e) {
e.preventDefault()
const username = e.currentTarget.elements['username'].value
if (!username) return
e.currentTarget.elements['username'].value = ''
const key = this.nextKey++;
this.users[key] = {
key: key,
name: username
};
this.recentlyAdded = this.users[key];
this.render();
},
handleListClick(e) {
const target = e.target;
if (target.classList.contains('remove')) {
this.handleRemoveUser(target.parentNode.id);
}
},
handleRemoveUser(key) {
delete this.users[key];
this.recentlyRemovedKey = key;
this.render();
}
}
Users.init().remove:before {
content: 'X';
}<form id="add-user-form">
<input type="text" placeholder="username" name="username">
<button>add User</button>
</form>
<hr>
<ul id="user-list"></ul>
<script id="user-template" type="text/html">
<li data-key="${key}" id="${key}">${name} <button class="remove"></button></li>
</script>edited 9 mins ago
answered Apr 9 '18 at 18:36
Sᴀᴍ OnᴇᴌᴀSᴀᴍ Onᴇᴌᴀ
10.6k62168
10.6k62168
add a comment |
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f190088%2fjavascript-module-to-render-and-handle-a-form-to-add-users%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown