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

Adds proper styles, Solves Issue #12 #262

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Fanksies
Copy link

@Fanksies Fanksies commented Oct 3, 2020

I noticed too late that someone else had already attempted to fix this issue before, however I noticed that the issue was still open and decided to give it a shot.

Here's a preview of how it looks in GIF form
https://i.imgur.com/PE9Eqjs.gif

Let me know what you think.

@rugk rugk linked an issue Oct 4, 2020 that may be closed by this pull request
@rugk
Copy link
Owner

rugk commented Oct 4, 2020

Hi @Fanksies,
thanks for your first contribution to this project! 🎉 👍 🏅
I hope you'll like this project and enjoy hacking on it… 😃

Your code and the solution looks very nice, thanks. I'll take a deeper look soon.
This supersedes #226 if merged.

BTW you can (automatically) let issues close when a PR is merged by adding some "magic" text to your PR body. (Manually linked it now.)

And another BTW: You can also upload GIFs/images on GitHub via drag & drop. 🙂

Copy link
Owner

@rugk rugk left a comment

Choose a reason for hiding this comment

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

Your solution looks nice, but I see one problem:
AFAIK this always displays the button on the next line… I'd better only want the button displayed on the next line, if it is needed/i.e. if the message text is long and spans two lines.

@@ -171,6 +171,20 @@ a:active {
margin-top: 8px;
}

#messageContrast {
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm #messageContrast is only one case where this might happen.
It would be best if the solution were message-agnostic, i.e. somehow integrate it into .message-box.

@Fanksies
Copy link
Author

Fanksies commented Oct 4, 2020

Thanks for the tips and the feedback, I sincerely appreciate them as they help me learn a lot in the process of contributing to open source projects.
You're correct about the button displaying on the next line only for the contrast warning message.
I understand that a message agnostic solution would be ideal as it would be cleaner and easier to reuse down the road.
I'll look into it!

@rugk
Copy link
Owner

rugk commented Aug 16, 2021

@Fanksies By the way… did you by chance found a solution or even just an idea here on how to solve this?

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.

Incorrect display of messages on multiple lines
2 participants