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

[client] fetchComments is adding another PostBox even if there is already one displayed #842

Closed
Kerumen opened this issue Apr 23, 2022 · 7 comments
Labels
bug client (Javascript) client code and CSS
Milestone

Comments

@Kerumen
Copy link

Kerumen commented Apr 23, 2022

Everytime we call window.Isso.fetchComments(), a new PostBox is added, even if there is already a PostBox displayed. It means that if we call it 4 times, there will be 4 PostBox displayed.

Screenshot 2022-04-23 at 15 43 20

@ix5 ix5 added client (Javascript) client code and CSS bug labels Apr 23, 2022
@ix5 ix5 added this to the 0.13 milestone Apr 23, 2022
@ix5 ix5 changed the title fetchComments is adding another PostBox even if there is already one displayed [client] fetchComments is adding another PostBox even if there is already one displayed Apr 23, 2022
@ix5
Copy link
Member

ix5 commented Apr 23, 2022

Recent changes made to embed.js which moved the logic of the postbox creation to after the /config endpoint had been fetched introduced this issue.

Currently, Isso promises the window.Isso.fetchComments() and window.Isso.init() functions to users in the docs.
Should we keep it that way? IMO those hooks make sense for SPA and the like, although I have to admit I have very little experience with the subject.

@Kerumen, would you like to tackle this issue? In the process, you might use the opportunity to untangle the JS a bit (there is a lot of logic inside embed.js and isso.js that might better be in separate files, afaics, and some require() dependency chains seem unnecessarily deep). I defer to your knowledge and experience there.

@ix5
Copy link
Member

ix5 commented Apr 23, 2022

Also, the styling to the comment box in the screenshot you attached looks very cool! Very elegant and understated.

Maybe you could share the CSS you used, and if people agree, we could make something like it (with better contrast than grey-on-grey text, and without the cool-but-too-whimsical submit button) the Isso default?

@ix5
Copy link
Member

ix5 commented Apr 23, 2022

@Kerumen I noticed that I still had #821 open. I changed the logic there and pushed just now, so maybe you could review that PR again, with the new changes.

@Kerumen
Copy link
Author

Kerumen commented Apr 24, 2022

Recent changes made to embed.js which moved the logic of the postbox creation to after the /config endpoint had been fetched introduced this issue.

Currently, Isso promises the window.Isso.fetchComments() and window.Isso.init() functions to users in the docs. Should we keep it that way? IMO those hooks make sense for SPA and the like, although I have to admit I have very little experience with the subject.

@Kerumen, would you like to tackle this issue? In the process, you might use the opportunity to untangle the JS a bit (there is a lot of logic inside embed.js and isso.js that might better be in separate files, afaics, and some require() dependency chains seem unnecessarily deep). I defer to your knowledge and experience there.

You are right about SPA. I do integrated Isso in a SPA and needed to call fetchComments while navigating between posts to load the new comments. Otherwise we were stuck with the comments from the previous post. I can have a look but I have literally no knowledge at all about the codebase and how to develop locally so it can take some time for me to tackle this.


Also, the styling to the comment box in the screenshot you attached looks very cool! Very elegant and understated.

Maybe you could share the CSS you used, and if people agree, we could make something like it (with better contrast than grey-on-grey text, and without the cool-but-too-whimsical submit button) the Isso default?

Yes of course! I can do a separate PR for this, this one is more in my capabilities 😄 (NB: I took the colors from the Tailwind Palette, I really like their default).

@ix5
Copy link
Member

ix5 commented Apr 24, 2022

Just quickly re:

I can have a look but I have literally no knowledge at all about the codebase and how to develop locally so it can take some time for me to tackle this.

  • Clone repo
  • make init js (will install npm packages and generate js bundles)
  • virtualenv .venv && source .venv/bin/activate && pip install -e .
  • isso -c share/isso-dev.cfg run
  • Visit http://localhost:8080/demo/index.html in browser

Then you can edit js files. With npm run watch-dev, you can run webpack automatically whenever a file changes. You will have to refresh to browser tab ofc, there's no "hot-reload" or whatever it's called.

Keep in mind ES5 compatibility (for now, I know it irks you but babel is just so heavy) and feel free to iron out any quirks/inconsistencies you see along the way. The upcoming 0.13 release is going to be shipping a few breaking changes, so feel free to go wild ;)

@Kerumen
Copy link
Author

Kerumen commented Apr 25, 2022

Thanks for the development steps. I quickly tried to launch the project yesterday and could not reproduce the bug. It seems that in development, window.Isso.fetchComments() reload the whole page. I have to dig more.

@ix5
Copy link
Member

ix5 commented Jun 5, 2022

Should mostly be fixed by #821
Still not entirely sure if any of that code is racy, but looks fine for now.

@ix5 ix5 closed this as completed Jun 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug client (Javascript) client code and CSS
Projects
None yet
Development

No branches or pull requests

2 participants