Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Dark theme follow ups round 2 #2659

Merged
merged 12 commits into from
Feb 19, 2019
Merged

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented Feb 18, 2019

Includes things like:

  • Tweak LLP color
  • (+) icons
  • room pills aren't styled
  • lightbox backgrounds
  • widget blending
  • markdown indicator
  • member info back icon
  • member search hard to read

image

Fixes various pieces of element-hq/element-web#8554
Fixes element-hq/element-web#8584
Fixes element-hq/element-web#8587
Fixes element-hq/element-web#8673
Fixes element-hq/element-web#8637

@jryans jryans requested a review from a team February 18, 2019 18:36
@jryans
Copy link
Collaborator Author

jryans commented Feb 18, 2019

(Push CI failure expected, I deleted the branch.)

@turt2live turt2live self-assigned this Feb 19, 2019
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

This generally looks okay, I think.

In future, please reference issues in the commits and provide a bit more detail in the commits for bulk PRs. It's nearly impossible to review the whole diff as one thing, thus somewhat forcing the reviewer to go commit by commit. Having the context of the issue and description of the change (as though it's a mini PR, in a way) helps make review easier. Alternatively, open up a bunch of smaller PRs with the same amount of detail.

@jryans
Copy link
Collaborator Author

jryans commented Feb 19, 2019

In future, please reference issues in the commits and provide a bit more detail in the commits for bulk PRs. It's nearly impossible to review the whole diff as one thing, thus somewhat forcing the reviewer to go commit by commit. Having the context of the issue and description of the change (as though it's a mini PR, in a way) helps make review easier. Alternatively, open up a bunch of smaller PRs with the same amount of detail.

Okay, will do! I normally prepare my PRs expecting people to look at each commit, as that's the way I am used to working and reviewing (with meaningful commits rather than 10 commits that say "fix"). However, it hasn't felt like people actually look at them when reviewing usually (since most other PR author's don't seem to expect that way of reviewing), so I've been a bit unsure if people would actually read the extra commit info, and as you can see here I didn't put much in each commit this time.

I guess the best plan is to clarify in a PR how it is best reviewed to avoid confusion.

Thanks for the review! 😁

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants