skyra-project / discord-components

Discord Webcomponents for real looking messages on the web
MIT License
243 stars 31 forks source link

Lit rewrite #337

Closed TheBv closed 11 months ago

TheBv commented 1 year ago

I saw some work being done on the lit branch again so I decided to whip up this draft before the same work gets done twice.

So far I've ported every component to lit. There are however some challenges which I'm not yet sure how to overcome:

Parent dependent Styling

Lit uses ShadowDOMs for each component which is causing issues with nested components that used to rely on the parents class for styling. E.g if you define the light-theme for discord-messages you can see that the name doesn't have the correct color: image

Similar issues plague the other components as well e.g replies don't render correctly as well. Although some of these components can probably be fixed by adjusting the css directly.

Icons

We used to style the icons in some components to match the size we needed. TemplateResults which I've used so far based on the previous components can't be styled. We can create function driven components but lit doesn't support spreading. So I see two solutions: A: use open-wc's lit-helpers or B: We define the function to take the various arguments we need to style the component.

Notes

Most of the visible issues one can see in the demo pretty much derive from the two issues mentioned above. If anyone has some good ideas/directions on how to tackle them I'd be willing to apply them.

Also I haven't implemented any of the various commits that have happened since the creation of this branch including changes done on the main branch.

There's a lot of cleanup that still needs to be done as well so please keep that in mind :)

favna commented 1 year ago

Havent looked in detail yet, but I can already say that using lit-helpers is fine by me.

favna commented 1 year ago

I rebased this branch on the base feat/rewrite-to-lit and added some other changes as well

favna commented 1 year ago

So I think I covered the issue with icons in my first reply? That just leaves the issue with parent-dependent styling. There are a few different types I think looking at the old code.

Scenario 1: .discord-message .discord-mention

In this scenario, the styling for discord-mention isn't really dependent on its parent. The reason .discord-message is there, is to ensure that the styling is scoped to only discord-mention that exists within a discord-message. So in this case this can just be changed to a single :host, as discord-mention is always a child of discord-message

Scenario 2: .discord-light-theme .discord-reaction

In this scenario, we want to know if the light theme is active or not and if it is applying some style overrides. This scenario is already covered in the rewritten discord-message component and doesn't change much.

I think that about covers it? If I'm forgetting a scenario let me know. Quite frankly I think I am forgetting one and it's one I asked help about in the Discord server way back but I never did anything with the info I got... sadly I cannot find it anymore either if so.

One final note for fixing these issues, and any others that may arise, we can also look at https://github.com/ing-bank/lion for how they solve problems, if they do at all. ING Bank is one of the big 3 banks in The Netherlands and they've been using webcomponents (and Lit) ever since its inception (thanks me attending keynote talks at Frontmania and TEQNation)


Lastly, regarding the CI, yeah I know it fails on the React bundle. All that stuff I still need to figure out how to do. Once all the components are working on the demo page I want to do still do the following in no particular order:


One final note, if you want to talk to me more directly that GH issue then I ask you to join the Skyra Lounge discord server: https://join.skyra.pw. I'll reply much faster there than here as I don't really check GitHub during work hours, but do answer small questions on Discord.

TheBv commented 1 year ago

Okay I've fixed most issues that I was mentioning now there's a couple more for which I don't know the answer to (or just want to ask about):

  1. DiscordSpoilers css assumes a lot of times that DiscordMessage is it's parent. Should we just allow this element to only work if DiscordMessage is it's parent?
  2. DiscordMessage styles the a and the styling gets lost in child components due to the shadow doms. How should we deal with that? My suggestion would be to export a "LinkStyling" that each component can then add to their own styling. But there probably are better solutions.
  3. DiscordMessage applies extra styling to various sub elements e.g strong. This styling also get's lost. I've been able to fix this like this although the issue with that solution is that it doesn't work for "normal" code elements. Maybe what I suggested in 2. would work here as well?
  4. DiscordMessage used to have this styling which didn't work correctly. I moved it to DiscordMessages but due to the shadow dom the styling can't be applied correctly. I don't know how to fix this.
  5. The badge icon in replies is ever so slightly larger new: image old: image I genuinely don't know why. The css seems to be exactly the same.

That about covers everything I've picked up on, I think. I'll focus on implementing all the various changes that happened to the components in the main branch next.

edit: One thing I forgot to mention was that the built in styling in the embed example doesn't work as well. I assume we need to import the styles to make these things work? I'm not sure how to do that though.

favna commented 1 year ago
  1. DiscordSpoilers css assumes a lot of times that DiscordMessage is it's parent. Should we just allow this element to only work if DiscordMessage is it's parent?

Yes

2. DiscordMessage styles the a and the styling gets lost in child components due to the shadow doms. How should we deal with that? My suggestion would be to export a "LinkStyling" that each component can then add to their own styling. But there probably are better solutions.

See below at 3

3. DiscordMessage applies extra styling to various sub elements e.g strong. This styling also get's lost. I've been able to fix this like this although the issue with that solution is that it doesn't work for "normal" code elements. Maybe what I suggested in 2. would work here as well?

I would rather suggest we force the usage of discord-inline-code and regular code elements don't get styled the same, so I'd say the current behaviour is desired.

Actually, on that note we should also do discord-link and not style regular a tags. Good thing is is a major version so we can break it to hell and back hehe.

For the rest I'm going to check out the PR again

favna commented 1 year ago

5. The badge icon in replies is ever so slightly larger new: image old: image I genuinely don't know why. The css seems to be exactly the same.

I just checked this through the dev tools and both with the new and the old it reports at 15x15 pixels. Switching between tabs also seems identical.

favna commented 1 year ago

A few things I notice when switching between tabs of the "No framework" example on codesandbox (use "open in new window" in codesandbox for a full view) and running yarn start in this branch:

  1. The timestamp was closer to the username before
  2. The orange of a mention doesn't show
  3. Compact mode paddings/margins seem to be wrong
  4. Replies and commands are all out of wack
  5. The timestamp in the embed is wrong
  6. Stuff breaks hard with Embed description built-in styling example but as addressed before I think we should just remove that support and force people to use the discord-* components.
  7. Some link text decorations are wrong but as addressed before, we should probably just create discord-link and forego styling a tags.
  8. Note that the incorrect styling of 7 also counts for Embed Title and Embed Author, but I think we can make those use discord-link after creating...?
Screenshots for 1 and 2 **New:** ![image](https://user-images.githubusercontent.com/4019718/236665613-28bc1b3f-00a3-4235-b0c8-77697f9485c8.png) **Old:** ![image](https://user-images.githubusercontent.com/4019718/236665620-36cca019-6383-4484-a552-13a236192dbe.png)
Screenshot for 3 **New:** image **Old:** image
Screenshot for 4 **New:** image **Old:** image
Screenshots for 5 **New:** image **Old:** image

edit: One thing I forgot to mention was that the built in styling in the embed example doesn't work as well. I assume we need to import the styles to make these things work? I'm not sure how to do that though.

Regarding this, I assume you're referring to the very weird styling here:

image

That is indeed peculiar. I'm also not sure how to fix it. Styles are quite all over the place there in the devtools inspector.

In general though I can say that we should use :host much more as opposed to wrapping the component in a single div for a class name, that has caused various issues such as 6db928b25de380d4cb82c8066b0f727bebcb5031 and ab993b06c145eee40a895445c01d7a32a13c8c17

TheBv commented 1 year ago

Okay I'm pretty certain I've addressed all the issues that you mentioned. I also went ahead and added a DiscordPre Element and generified DiscordCode so it supports multiline code blocks as well light theming and embed support.

One caveat: Elements that are styled differently in an embed (e.g. DiscordCode) don't get the embed styling automatically. The only way to achieve that would be to iterate through all of the elements parents until it finds one that is an embed component, or lightThemed. I decided not to do that though so the user has to define those properties themselves. One could add them later though.

Also should we rename DiscordCustomEmoji to DiscordEmoji I feel like it'd make more sense given our current naming convention.

favna commented 1 year ago

Something weird is happening with ephemeral replies. I can't quite find out what and why: image

Also in non ephemeral with compact mode the text doesn't center out correctly with the username: image

favna commented 11 months ago

@TheBv I forced your branch changes onto feat/rewrite-to-lit because it was a merge conflict hellish clusterfuck to do it properly so your PR will show up as merged but ofc in reality it's not yet done, so we'll have to continue on a new PR when you got time. I have also created https://github.com/skyra-project/discord-components/pull/367 as placeholder PR for tracking what will go into the main branch, though eventually for that also I will just force the commits onto main when done.