Add Hovercards #75
Add Hovercards #75
Conversation
vscode didn't catch these somehow???
|
notices problem immediately after opening PR also the banners on glicht soc are broken :c |
There was a problem hiding this comment.
Tried it and it looks good! But I wonder if we can get rid of the remaining inline CSS files and just use proper CSS classes for that. It would align with the current proplates and should be relatively easy to translate, I guess.
Also, regarding the "show more"/"show less" button. I don't know what would be the best way of managing translations for that, but I also think we should have them. What do you think?
| } | ||
|
|
||
| export function isLogging() { | ||
| return settings.logging; | ||
| } | ||
|
|
||
| export function statusVisibility() { | ||
| return settings.statusVisibility; | ||
| return settings.proplate.statusVisibility; |
There was a problem hiding this comment.
would it be useful (to introduce) a version marker of some sort for settings in general?
not for new settings key which can use the default value but for such structural changes to allow migration.
maybe versioning the settings is too much for KISS reasons for now.
in this case we can detect and migrate without it, or let migration out and so users would loose their settings this time. (shouldn't happen too often)
There was a problem hiding this comment.
while i think it may be a little annoying for users to have to re-set their settings from time to time we don't push updates that change the structure of things that often. and also i'm lazy '^^
I'll gladly review a PR for it, but admittedly oftentimes i just wanna have the shiny new feature :3c
|
regarding your comment @nachtjasmin do you mean setting the positions via css variables instead of inline? by translation do you mean language translation or positional translation? cause i don't see why we would need to translate the text in our addon - and if so why start with this one specific button? |
|
don't remember why I wrote that, I think, it was related to the gradient and all that static stuff. And I meant language translation, but you're right, why start now. Let's keep it simple. ^^ |
ItsVipra
force-pushed
the
hovercard.js
branch
from
18b43f9
to
6dc1911
Compare
| try { | ||
| cacheResult = await storage.local.get(); | ||
| if (!cacheResult.hovercardCache) { | ||
| //if result doesn't have "pronounsCache" create it |
There was a problem hiding this comment.
| //if result doesn't have "pronounsCache" create it | |
| //if result doesn't have "hovercardCache" create it |
| if ( | ||
| profile && | ||
| Date.now() - timestamp < cacheMaxAge && | ||
| hostname == location.host && |
There was a problem hiding this comment.
what if I'm on two instances and open the hovercard for the same account? looks like they constantly overwrite each other than, so location.host should be included in the cache-key, eg. location.host+";"+accountname (for a flat cache)?
| @@ -59,6 +59,32 @@ export async function fetchPronouns(dataID, accountName, type) { | |||
| return pronouns; | |||
| } | |||
|
|
|||
| export async function fetchProfile(dataID, accountName) { | |||
There was a problem hiding this comment.
add js doc (like for fetchPronouns and the other functions in this file)
| if (cacheResult) return cacheResult; | ||
|
|
||
| if (!dataID) { | ||
| warn(`Could not fetch profile for user ${accountName}, because no status ID was passed.`); |
There was a problem hiding this comment.
status ID or data ID? (see also same nameing in fetchPronouns)
| timestamp: Date.now(), | ||
| profile: profile, | ||
| hostname: location.host, | ||
| version: currentVersion, |
There was a problem hiding this comment.
I'm not sure that there's a benefit from saving the version separately on each entry instead of for the whole cache and trash all of it in one go if version mismatch. (at least in the latter case the cache don't grow forever (no cache evict) but resets on version changes)
(factors include how the lifecycle of the extension works in regards to startup and ext update, not in my head right now)
(undecided which/if a approach has a clear advantage right now)
| }, listenerTimeout); | ||
| }); | ||
|
|
||
| nametag.addEventListener("mouseleave", () => { |
There was a problem hiding this comment.
not sure, did you try to enter the nametag a little bit and then move the cursor out of it on the same path?
can you get the hovercard stuck with that?
(hinting of race between mouseleave and the timeout-function in mouseenter; resolve via saving timeoutID and calling clearTimeout)
There was a problem hiding this comment.
knowing of the existence of clearTimeout helps a lot, thanks :D
| nametag.addEventListener("mouseleave", () => { | ||
| hovering = null; | ||
| setTimeout(() => { | ||
| hovercardExists = false; |
There was a problem hiding this comment.
what does hovercardExists mean?
if you hover again over the same nametag, it won't remove the hoverCard, but hovercardExists was already set to false. (no mental capacity right now to go over each possible combination of hovering and hoverCardExists)
(I'm not sure if grasp the intention right)
src/libs/hovercard/hovercard.js
Outdated
| hovering = false; | ||
| hovercardExists = false; | ||
| setTimeout(() => { | ||
| if (hovering) return; | ||
| removeHoverCard(hovercard); | ||
| }, listenerTimeout); |
There was a problem hiding this comment.
see above for hovercardExists, and why is it here outside of setTimeout? also hovering was set to null, not false in the other event listener.
seems fishy/inconsistent.
| */ | ||
| function removeHoverCard(card) { | ||
| if (layer.getAttribute("noremove")) return; //debug settings | ||
| card.classList.add("hidden"); //TODO: interrupt removal by hovering again |
There was a problem hiding this comment.
this todo statement makes even less sense after reading the setTimeout-callback's inside the mouseleave
| function setMore() { | ||
| bio.style.maxHeight = "25vh"; | ||
| span.textContent = "Show more"; | ||
|
|
||
| showMoreDiv.style.marginTop = (-showMoreDiv.getBoundingClientRect().height).toString() + "px"; | ||
| showMoreDiv.style.backgroundImage = | ||
| "radial-gradient(farthest-side at bottom center, var(--background-color) 0%, var(--background-color) 75%, transparent)"; | ||
| showMoreDiv.style.paddingTop = ".25em"; | ||
| showMoreDiv.style.paddingBottom = "0px"; | ||
|
|
||
| showMore.addEventListener("click", () => setLess(), { once: true }); | ||
|
|
||
| moveOnScreen(hovercard); | ||
| } | ||
|
|
||
| function setLess() { | ||
| bio.style.maxHeight = "unset"; | ||
| span.textContent = "Show less"; | ||
|
|
||
| showMoreDiv.style.marginTop = "0px"; | ||
| showMoreDiv.style.backgroundImage = | ||
| "radial-gradient(farthest-side at top center, var(--background-color) 0%, var(--background-color) 75%, transparent)"; | ||
| showMoreDiv.style.paddingTop = "0px"; | ||
| showMoreDiv.style.paddingBottom = ".25em"; |
There was a problem hiding this comment.
multiple static css values? -> hovercard.css ?
There was a problem hiding this comment.
Besides the comments by @se4598, I'm fine with the changes. lgtm.
|
I procrastined this so long that it's now an official feature '^^ |
This PR adds the option to have a mini version of a user's profile show up on hover. Styling is mostly copied from the instance, with a few manual tweaks. Exactly what is shown in the profile is customizable. Additionally, profiles are cached for 24h as to reduce both API requests and load times.
Currently only works in the home timeline and not on expanded posts.
Has not been tested in advanced mode or glitch-soc much (help is appreciated).