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

pageWrapper should be initialized in halfmoonOnDOMContentLoaded #20

Closed
visoft opened this issue Aug 14, 2020 · 5 comments
Closed

pageWrapper should be initialized in halfmoonOnDOMContentLoaded #20

visoft opened this issue Aug 14, 2020 · 5 comments

Comments

@visoft
Copy link
Contributor

visoft commented Aug 14, 2020

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
Copy link
Owner

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: #16 (comment)

@visoft
Copy link
Contributor Author

visoft commented Aug 14, 2020

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
Copy link
Owner

halfmoonui commented Aug 15, 2020

@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
Copy link
Owner

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
Copy link
Contributor Author

visoft commented Aug 17, 2020

Works great! Thanks for the quick fix!

@visoft visoft closed this as completed Aug 17, 2020
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

2 participants