halfmoonui / halfmoon

Halfmoon is a highly customizable, drop-in Bootstrap replacement. It comes with three built-in core themes, with dark mode support for all themes and components.
https://www.gethalfmoon.com
MIT License
3.04k stars 118 forks source link

pageWrapper should be initialized in halfmoonOnDOMContentLoaded #20

Closed visoft closed 4 years ago

visoft commented 4 years ago

I've installed the npm package and am using it with React as shown in the documentation. There is an issue though. I've called halfmoonOnDOMContentLoaded, however that function doesn't initialize pageWrapper or stickyAlerts like it does for darkModeOn.

I can put in a PR fix this, however how is the npm package built given there is no package.json in the project? Should I just alter the halfmoon.js file and will you create a new minified version and the npm package?

halfmoonui commented 4 years ago

The halfmoonOnDOMContentLoaded() function is not really meant to initialize the page wrapper or the sticky alerts. It instead initializes dropdowns, keydown event handler, click event handler, shortcuts, file inputs, and the dark-mode cookies. The page wrapper and the sticky alerts container therefore need to be defined inside your template.

However, after reading your comment, I think there could be a use case for the function to actually create these components inside the page automatically. For that, I would probably go with an extra parameter that can be passed onto the function. However, that could lead to some other issues. I will think about this a bit more.

Regarding the package.json, I explained it here: https://github.com/halfmoonui/halfmoon/issues/16#issuecomment-673852837

visoft commented 4 years ago

Basically my issue is that pageWrapper is undefined when it goes to toggle the sidebar. This is definitely a problem with virtual DOM as the elements aren't available when the framework is initialized. I corrected it locally by the following code inside halfmoonOnDOMContentLoaded(), but I see your point about that function:

    if (!halfmoon.pageWrapper) {
      halfmoon.pageWrapper = document.getElementsByClassName("page-wrapper")[0];
    }
    if (!halfmoon.stickyAlerts) {
      halfmoon.stickyAlerts: document.getElementsByClassName("sticky-alerts")[0];
    }

I'm not sure of a best approach for this. I'll be interested to hear your ideas.

halfmoonui commented 4 years ago

@visoft I am sorry, I misunderstood what you meant before. I think you have found a bug, because the functions would fail in cases where a virtual DOM is being used, and the pageWrapper is not properly initialized. Your solution is probably the best one too. Its not elegant, but it works. I will patch this out ASAP. Thank you so much, really appreciate you bringing this up!

halfmoonui commented 4 years ago

Hey I implemented your fix and patched the issue with a new release: https://github.com/halfmoonui/halfmoon/releases/tag/v1.0.4. Please let me know if everything works for you as well, I have tested myself and everything seems to be running properly.

visoft commented 4 years ago

Works great! Thanks for the quick fix!