-
Notifications
You must be signed in to change notification settings - Fork 438
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] Highlight linked comments to identify them #845
Conversation
cdd1c1c
to
b0da339
Compare
@ix5 Can you take a look at this please? I have CSS added to set a background color included. I don't think that separating it out like what we did in #844 makes sense because this feature should be included by default. It's also different since it doesn't require any configuration by the site owner to function. Also, I did figure out how to get it not to conflict with #844. By not setting the 100% keyframe, it automatically just fades out to whatever background color is the fallback for the element. Let me know if there's a better place for the |
Hey @BBaoVanC, I'm off to a well-deserved break, starting tomorrow. So I won't be able to review your changes for about a week. Let's do a deal: You figure out a solution and submit draft PRs to at least some of these open issues which I think you're more than capable of solving:
... and I'll be happy to take some time to review this highlighting PR once I get back ;-) Would also be nice if at some of these low hanging fruits could be fixed: |
I'll take a look at those. Hope you have a nice break! |
daef1d8
to
fdcadad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have CSS added to set a background color included. I don't think that separating it out like what we did in #844 makes sense because this feature should be included by default. It's also different since it doesn't require any configuration by the site owner to function.
That's fine. As commented inline, the yellow is a bit jarring at night, but yellow is universally accepted as a marker/highlight/attention color. Maybe once there's a dark mode someone can style it differently. Keep as-is for now.
Let me know if there's a better place for the hashchange event listener, and if window.addEventListener is the right function to use for it; I wasn't really sure what to do with it.
I'm not qualified to comment on event listeners, my JS knowledge is pretty bad. I found this SO answer which seems to suggest a similar thing. Availability of Caniuse: hashchange seems good.
As for stuffing this piece of code into init()
: IMO that file is already quite overloaded. Maybe shoving everything related to timers and events into a separate file (that can be stubbed/mocked) would simplify testing a good bit.
Several places rely on window.location.*
. Unifying that would be a nice first step.
E.g. this is just a stupid duplication:
https://github.com/posativ/isso/blob/fdcadaddcd87fe97fbd7ca328a2ef7ea56eccb87/isso/js/app/api.js#L7
https://github.com/posativ/isso/blob/fdcadaddcd87fe97fbd7ca328a2ef7ea56eccb87/isso/js/embed.js#L74
1c59e03
to
a6ac9e5
Compare
Would you mind adding a small styling guide to the docs? (Perhaps under "Tips & Tricks", 1-2 lines should be enough). Code-wise, this looks good right now. |
a6ac9e5
to
ee80a3a
Compare
I added some docs, now all that's left is a small test if possible which just makes sure that |
Remove inserted comments again so that following tests have a blank DB again.
Added some tests. Thank you and looking forward to reviewing other great contributions! |
Fixes #838
TODO:
.isso-target
gets applied properly)