Cleanup

[rimlin]

#270
Tasks 2, 4, 5

[tjoskar]

Thanks @rimlin!
Take a look at my comments and say what you think.

[tjoskar]

I'm not sure about the name :) What do you think of isWindowDefined() or isBrowserContext()?

[sapierens]

I vote for isWindowDefined() 👍

[tjoskar]

I think it is better to move down this line to where it is used (~82), now when it is so simple.

[tjoskar]

Why checking against false and not just: !isWindowExists()?

[rimlin]

I was prefer it, because this approach is more explicit as i think, but if it not acceptable - no problem

[tjoskar]

This is much better but I still think that we should create a function that is only responsible for setting and removing css class names. Something like:

function removeCssClassName(element: HTMLImageElement | HTMLDivElement, cssClassName: string) {
  element.className = element.className.replace(cssClassName, '');
}

function addCssClassName(element: HTMLImageElement | HTMLDivElement, cssClassName: string) {
  if (!element.className.includes(cssClassName)) {
    element.className += `  ${cssClassName}`
  }
}

And then use these two function instead of:

if (element.className && element.className.includes(cssClassNames.loaded)) {
  element.className = element.className.replace(cssClassNames.loaded, '');
}

element.className += ' ' + cssClassNames.failed;

setLoadedStyle

[tjoskar]

@rimlin, I saw your comment now, regarding that you addressed 2, 4, 5 and not 3 which this comment is about. I will therefore accept this PR without this change.

[tjoskar]

Is there an extra white line above import { isWindowExists } from './utils';? Can you remove it :)

[tjoskar]

What do you think of creating a new file, calling constants and place cssClassNames inside it?

[tjoskar]

Thanks @rimlin