Skip to content

Commit

Permalink
Refactor to Switch Away from Using innerHTML
Browse files Browse the repository at this point in the history
It's bad practice to set `innerHTML` as this has a high performance cost. 

Instead of using `innerHTML` we switch to use `textContent` when the only thing being replaced is the text. When we insert multiple children nodes to the element (button), we now use the helper functions `getIndicatorHolder` and `getTextSpan` to separately generate the HTML Elements needed to insert as a child element to the button by using `insertAdjacentElement`.
  • Loading branch information
rgomezp committed Aug 12, 2020
1 parent dc1c5eb commit 468243d
Showing 1 changed file with 17 additions and 11 deletions.
28 changes: 17 additions & 11 deletions src/slidedown/Slidedown.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,14 +143,18 @@ export default class Slidedown {

if (state) {
// note: savingButtonText is hardcoded in constructor. TODO: pull from config & set defaults for future release
this.allowButton.innerHTML = this.getIndicatorHolderHtmlWithText(this.categoryOptions.savingButtonText).innerHTML;
this.allowButton.disabled = true;
this.allowButton.textContent = null;

this.allowButton.insertAdjacentElement('beforeend', this.getTextSpan(this.categoryOptions.savingButtonText));
this.allowButton.insertAdjacentElement('beforeend', this.getIndicatorHolder());

addDomElement(this.buttonIndicatorHolder,'beforeend', getLoadingIndicatorWithColor(COLORS.whiteLoadingIndicator));
addCssClass(this.allowButton, 'disabled');
addCssClass(this.allowButton, SLIDEDOWN_CSS_CLASSES.savingStateButton);
} else {
// positiveUpdateButton should be defined as written in MainHelper.getSlidedownPermissionMessageOptions
this.allowButton.innerHTML = this.categoryOptions.positiveUpdateButton!;
this.allowButton.textContent = this.categoryOptions.positiveUpdateButton;
removeDomElement(`#${SLIDEDOWN_CSS_CLASSES.buttonIndicatorHolder}`);
this.allowButton.disabled = false;
removeCssClass(this.allowButton, 'disabled');
Expand All @@ -166,7 +170,10 @@ export default class Slidedown {

if (state) {
// note: errorButtonText is hardcoded in constructor. TODO: pull from config & set defaults for future release
this.allowButton.innerHTML = this.getIndicatorHolderHtmlWithText(this.categoryOptions.errorButtonText).innerHTML;
this.allowButton.textContent = null;
this.allowButton.insertAdjacentElement('beforeend', this.getTextSpan(this.categoryOptions.errorButtonText));
this.allowButton.insertAdjacentElement('beforeend', this.getIndicatorHolder());

addDomElement(this.buttonIndicatorHolder, 'beforeend', getRetryIndicator());
addCssClass(this.allowButton, 'onesignal-error-state-button');
} else {
Expand All @@ -181,18 +188,17 @@ export default class Slidedown {
return getPlatformNotificationIcon(this.notificationIcons);
}

getIndicatorHolderHtmlWithText(text: string): Element {
getIndicatorHolder(): Element {
const indicatorHolder = document.createElement("div");
const textHolder = document.createElement("span");
const divWrapper = document.createElement("div");

indicatorHolder.id = SLIDEDOWN_CSS_IDS.buttonIndicatorHolder;
textHolder.textContent = text;
addCssClass(indicatorHolder, SLIDEDOWN_CSS_CLASSES.buttonIndicatorHolder);
return indicatorHolder;
}

divWrapper.appendChild(textHolder);
divWrapper.appendChild(indicatorHolder);
return divWrapper;
getTextSpan(text: string): Element {
const textHolder = document.createElement("span");
textHolder.textContent = text;
return textHolder;
}

get container() {
Expand Down

0 comments on commit 468243d

Please sign in to comment.