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

fix: styling of components #72

Merged
merged 1 commit into from
Apr 23, 2021
Merged

fix: styling of components #72

merged 1 commit into from
Apr 23, 2021

Conversation

iCrawl
Copy link
Contributor

@iCrawl iCrawl commented Apr 22, 2021

This fixes a whole slew of CSS styling issues to get closer to Discords design philosophy.

Edit: I also took the liberty of copying out the settings and extensions to their own settings.json and extensions.json since I don't use Docker/Containers/Codespaces to work on this.

closes #42
closes #43

Old:
image

New:
image

Old:
image

New:
image

Old:
image

New:
image

Old:
image

New:
image

Old:
image

New:
image

@iCrawl iCrawl requested a review from favna as a code owner April 22, 2021 22:43
Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good, not sure about the .devcontainer changes, but if this PR is tested in both PC and tablet/phone, it LGTM.

@@ -145,7 +145,11 @@ export class DiscordEmbed implements ComponentInterface {
<slot></slot>
</div>
<slot name="fields"></slot>
<div class={clsx('discord-embed-media', { 'discord-embed-media-video': Boolean(this.video) })}>{this.renderMedia()}</div>
{this.image || this.video ? (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this execute as (this.image || this.video) ? (...) : null or as this.image || (this.video ? (...) : null)?

If ESLint doesn't complain, I think it might be worth to wrap the OR condition with parens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This executes as the former based on comparison operations.

Prettier does complain however, so I think we are on the safe side with this:
image

@iCrawl
Copy link
Contributor Author

iCrawl commented Apr 22, 2021

Devcontainer was not touched, I simply pulled out the settings and extensions so they are also applied when not using a container.

Edit:
Oops, actually yes, they need to be in the .vscode folder. I will be moving those.

Copy link
Member

@favna favna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than this it LGTM.

Should note btw that before I can release it I need to dive into the lib again because before my holiday late march I was struggling with something that was broken, but I don't remember what it was. That's also why the latest version is 2.0.3-alpha.0, because yarn link was being a huge PITA at that time.

@@ -0,0 +1,19 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this file and .vscode/settings.json. While they're not bad per se, it's just not something we really add in Skyra Project repos.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can do that. I mentioned it briefly to @kyranet though, so maybe you can find some nice solution for this overall.

If I would have coded this with Codespaces, it would have used his .devcontainer settings and forced it on me this way
I only added it since there is stuff like import sorting in there, which is kind of a source changing thing and will cause a conflict when you would re-save the file having those options enabled on your local settings

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not wrong. So far this hasn't been an issue though, so for now lets just revert it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed all unrelated changes 👍

@kyranet kyranet requested a review from favna April 23, 2021 18:47
@favna favna merged commit b440c2f into skyra-project:main Apr 23, 2021
@iCrawl iCrawl deleted the fix/styling branch April 23, 2021 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: codeblock text overflow on long lines bug: inline codeblocks don't have the correct background color
3 participants