Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A couple of suggestions #5

Open
danimoh opened this issue Nov 27, 2021 · 0 comments
Open

A couple of suggestions #5

danimoh opened this issue Nov 27, 2021 · 0 comments

Comments

@danimoh
Copy link

danimoh commented Nov 27, 2021

Hey Max,
while checking-out your project, I noticed some things that could be improved.
If you want, I can submit a pull request for these things.

let content = document.createElement('div');
let title = document.createElement('div');

Can be const

content.innerHTML = this.info.title;

title.innerHTML = this.info.title;?

if (this.info.link) {
if (this.info.link.href) {

Can be joined into a single if.

let description;
description = document.createElement('div');

Can be simplified to const description = ...

Object.assign(link.style, this.styles.link);

Should be moved to into the block where the link is created.

let notification = this.notification;

Can be simplified to const notification = .... Or just this.notification can be used.

setTimeout(function () {
notification.outerHTML = "";;
}, this.info.animation_duration * 1000 + 100);

setTimeout(() => notification.remove(), this.info.animation_duration * 1000 + 100)

let body = document.getElementsByTagName('body')[0];
body.append(notification_panel);

Can use document.body

let anim_t = this.info.animation_duration * 1000;

The animation duration is configurable, however the opacity transition duration not adapted to this.info.animation_duration

setTimeout(function () {
notificationContainer.style.opacity = 0;
}, display_t);
//destroy
setTimeout(function () {
notificationContainer.remove();
}, display_t + anim_t + 100);

The hiding logic could be moved into a hide method, which can then also be used in the close icon event listener.

if (!this.info.debug) this.info.debug = true;

Makes the flag kind of unnecessary 😄

if (this.info.link) {
if (!this.info.link.href)
if (this.info.debug)

These ifs can be joined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant