Repository navigation
fix multiple ui and content issues - #377
mostafaNazari702 wants to merge 0 commit into
Conversation
| transform: translateX(-50%); | ||
| margin-bottom: 8px; | ||
| padding: 0.75rem 1rem; | ||
| /* Fixed escapes overflow:hidden on any ancestor (e.g. archive collapse container) |
There was a problem hiding this comment.
I dont think this comment is supposed to be here
There was a problem hiding this comment.
I was certain that i needed to explain the big changes in the file and the removal of many css properties but will be removed.
| announcement.archived_at ? relativeTime(announcement.archived_at) : '' | ||
| ); | ||
|
|
||
| let textContent = $derived( |
There was a problem hiding this comment.
so the card only shows the actual readable text instead of hidden website formatting/code from the announcement card in the announcements page.
There was a problem hiding this comment.
Before i fixed it, it was like this on the card:
li { font-size: initial; font-weight: inherit; } pre { overflow: auto; padding: 1rem; border-radius: 0.25rem; background: var(--surface-four); }
Ushie
left a comment
Ushie
left a comment
There was a problem hiding this comment.
Forgot to submit my reviews, gg
There was a problem hiding this comment.
Use a package like SVooltip, a custom ToolTip implementation is not necessary
There was a problem hiding this comment.
SVooltip do not support interactive tooltips (hovering into the tooltip to click links/buttons inside it). From their own docs it says: "Including support for hovering the tooltip and clicking buttons inside and all that jazz. This will give you a tooltip. That's it."
We use interactive mode in TeamMemberCard to show a GPG key ID as a clickable link inside the tooltip. SVooltip cannot handle this, if my knowledge is at its peak. It also doesn't support svelte snippets for structured tooltip comtent.
The custom component is around 90 lines of logic + CSS, uses position: fixed麓 to escape ancestor overflow:hidden` (which was the original bug), and integrates with the project's CSS variables. It's small and purpose-built for exactly our requirements.
There was a problem hiding this comment.
You dont need the GPG Key ID to be a clickable link, the icon itself should be clickable, as it was pre-rewrite
There was a problem hiding this comment.
but it doesnt hurt to have the id to be clickabe too
There was a problem hiding this comment.
With this much code and pretty much no real usecase I'd rather use Svooltip
There was a problem hiding this comment.
You dont need the GPG Key ID to be a clickable link, the icon itself should be clickable, as it was pre-rewrite
TeamMemberCard.svelte has zero changes in this PR. the interactive tooltip with the clickable GPG key link existed before this PR in the rewrite, identically. This PR only changed the ToolTip's positioning from position: absolute to position: fixed + JS to fix tooltips being clipped by overflow:hidden on the archive collapse container.
There was a problem hiding this comment.
With this much code and pretty much no real usecase I'd rather use Svooltip
The code increase in this PR is the JS positioning logic (80 lines~) that fixes tooltips being clipped inside the archive collapse container (overflow:hidden). The old position: absolute approach couldn't escape ancestor clipping, that was the bug.
Adopting SVooltip is a separate discussion from this bugfix PR. It would require removing the interactive tooltip that was designed in the rewrite, refactoring 3 cosumer components, and adding a new dependency. Happy to discuss that as a separate issue if (OsumAtrix/$) wants to evaluate it, but bunndling it into this bugfix would expand the scope significantlly.
There was a problem hiding this comment.
It's fine I will do it myself later
There was a problem hiding this comment.
It's fine I will do it myself later
You can do that but i, from my experience, can assure you that you will get a ton of bugs. The code will also become significantly more to fix each bug individually. I do however wish you best of luck!
| let textContent = $derived( | ||
| announcement.content | ||
| ? announcement.content | ||
| .replace(/<style[^>]*>[\s\S]*?<\/style>/gi, '') | ||
| .replace(/<script[^>]*>[\s\S]*?<\/script>/gi, '') | ||
| .replace(/<[^>]*>/g, ' ') | ||
| .replace(/\s+/g, ' ') | ||
| .trim() | ||
| : '' | ||
| ); |
There was a problem hiding this comment.
What issue does this solve? Styling was intended for the preview, the CSS should be adjusted for it not to be as jarring
ba1c8fd to
e3a68c4
Compare
No description provided.