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.

Sign up for GitHub

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

Jump to bottom

Replace current red-text highlight with a full background-color highlight #2526

Merged
merged 1 commit into from
Jun 23, 2018
Merged

Replace current red-text highlight with a full background-color highlight #2526

merged 1 commit into from
Jun 23, 2018

Conversation

astorije
Copy link
Member

@astorije astorije commented Jun 8, 2018 •
edited
Loading

⚠️ This PR is currently set after my branch at #2315 so Do Not Merge until that other PR makes it in, but making it its own PR to facilitate the review.
All good now.

Supersedes #1847 and #1846.

Inspired by @xPaw's approach at #1847 (comment).
I am using colors given by https://color.adobe.com/ to match our main color to match it with our actual "new" blue.

Default theme:

screen shot 2018-06-04 at 00 37 44

Morning theme:

screen shot 2018-06-04 at 00 44 08

@astorije astorije added Type: Feature Tickets that describe a desired feature or PRs that add them to the project. Meta: Do Not Merge This PR should not be merged. labels Jun 8, 2018
@astorije astorije added this to the 3.0.0 milestone Jun 8, 2018
This was referenced Jun 8, 2018
Closed
Closed
@xPaw
Copy link
Member

xPaw commented Jun 8, 2018

Is it actually blocked on that PR? The commit that changes highlights doesn't seem to conflict. You just brought in 2 extra commits that are unrelated.

@astorije astorije force-pushed the astorije/higlight-border-and-bg branch from 41b2ae6 to 118866c Compare June 9, 2018 05:41
@astorije
Copy link
Member Author

astorije commented Jun 9, 2018

Is it actually blocked on that PR? The commit that changes highlights doesn't seem to conflict.

Yeah I get a few conflicts that I can't be bothered to fix. I'm hoping we can be done with #2315 in a couple days so this one can make it then.

You just brought in 2 extra commits that are unrelated.

That's because I rebase like an idiot. Fixed.

@AlMcKinlay AlMcKinlay mentioned this pull request Jun 10, 2018
Open
@astorije astorije force-pushed the astorije/higlight-border-and-bg branch from 118866c to d2718ea Compare June 10, 2018 18:17
@astorije astorije force-pushed the astorije/higlight-border-and-bg branch from d2718ea to d8719c8 Compare June 10, 2018 20:12
@astorije astorije force-pushed the astorije/higlight-border-and-bg branch from d8719c8 to 608cf93 Compare June 14, 2018 04:10
@astorije astorije force-pushed the astorije/higlight-border-and-bg branch from 608cf93 to 525cc56 Compare June 14, 2018 23:01
astorije
astorije commented Jun 18, 2018
View reviewed changes
client/css/style.css Outdated
color: #e74c3c;
}

#chat .message.highlight {
Copy link
Member Author

Choose a reason for hiding this comment

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

I realized that I accidentally removed .channel and therefore this highlights in queries too. I was about to re-add it, then wondered if .highlight class should be in queries at all. Any insight, @xPaw?

Copy link
Member

Choose a reason for hiding this comment

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

Keep the .channel.

All query messages are highlights, we don't want to style them.

@astorije astorije force-pushed the astorije/higlight-border-and-bg branch from 525cc56 to 0592ec3 Compare June 19, 2018 04:28
@astorije astorije changed the base branch from astorije/ui to master June 19, 2018 04:28
@astorije astorije self-assigned this Jun 19, 2018
@xPaw xPaw removed the Meta: Do Not Merge This PR should not be merged. label Jun 20, 2018
@xPaw
Copy link
Member

xPaw commented Jun 20, 2018

@astorije On mobile view there's padding around highlights. Do we want to remove these (I did that in Solarized).

@astorije astorije force-pushed the astorije/higlight-border-and-bg branch from 0592ec3 to 805b352 Compare June 21, 2018 05:36
@astorije astorije mentioned this pull request Jun 21, 2018
Open
@astorije
Copy link
Member Author

@xPaw, fixed the highlights in queries, and removed the paddings on mobile. Should be good to go.

xPaw
xPaw reviewed Jun 21, 2018
View reviewed changes
client/css/style.css Outdated
}

#chat .channel .message.highlight .from::after {
background: transparent;
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed. It was a relic from https://github.com/thelounge/thelounge/pull/2270/files#diff-97db1f70168fb5f12457b238ff6052b5L2230, thanks for noticing!

@astorije astorije force-pushed the astorije/higlight-border-and-bg branch from 805b352 to c2ce562 Compare June 21, 2018 17:30
@astorije astorije removed their assignment Jun 22, 2018
@astorije astorije requested a review from xPaw June 22, 2018 00:59
@astorije astorije merged commit 1d6c6dd into master Jun 23, 2018
@astorije astorije deleted the astorije/higlight-border-and-bg branch June 23, 2018 18:20
@astorije astorije mentioned this pull request Jun 23, 2018
Merged
@xPaw xPaw mentioned this pull request Jan 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xPaw xPaw xPaw approved these changes

Assignees
No one assigned
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@astorije @xPaw