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

chore: rename night mode to dark theme #5214

Closed
wants to merge 1 commit into from

Conversation

Udit-takkar
Copy link

#5169
Renamed all the files to dark theme .

Last inactive open PR #5170.
I have run all the tests and also changed some more files that were not addressed in last PR

@Udit-takkar
Copy link
Author

It is ready to be reviewed @chrisbobbe

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Jan 31, 2022

src/api/initialDataTypes.js has our Flow types that represent the data we expect to receive from servers. We should only change those types if

  • they've been doing their job badly (they were written without understanding what servers are actually sending)
  • new servers have started sending different data, and we want to start representing the new data.

I don't think either of those applies here. (If that's wrong, please talk about why you think so. 🙂) So we should leave those types unchanged, and Flow will lead you to undo several follow-on changes you made. Thanks!

@Udit-takkar
Copy link
Author

Udit-takkar commented Feb 1, 2022

src/api/initialDataTypes.js has our Flow types that represent the data we expect to receive from servers. We should only change those types if

  • they've been doing their job badly (they were written without understanding what servers are actually sending)
  • new servers have started sending different data, and we want to start representing the new data.

I don't think either of those applies here. (If that's wrong, please talk about why you think so. 🙂) So we should leave those types unchanged, and Flow will lead you to undo several follow-on changes you made. Thanks!

Okay you are right. Thanks for reviewing

@Udit-takkar
Copy link
Author

@chrisbobbe is it fine now?

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks @Udit-takkar! Some comments below.

Also please take a look at the Zulip project's guide on how to make a clean commit history:
https://zulip.readthedocs.io/en/latest/contributing/version-control.html

In particular, rather than having one commit with a bug and a commit later in the PR that fixes it, we ask that you squash the fix to make one commit that doesn't have the bug. So in this PR, your three commits so far should be squashed together as one commit.

@@ -1394,7 +1394,7 @@ Bugfixes and other improvements for your Zulip experience.
### Highlights for users (since 26.1.124 / 26.2.125)

* Highlight colors for code blocks now match the webapp and
offer more contrast, especially in night mode.
offer more contrast, especially in dark theme.
Copy link
Member

Choose a reason for hiding this comment

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

This is a historical changelog entry (from 2019), so let's leave it in the form we used at the time.

Copy link
Author

Choose a reason for hiding this comment

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

Okay

src/start/IosCompliantAppleAuthButton/Custom.js Outdated Show resolved Hide resolved
src/webview/css/css.js Outdated Show resolved Hide resolved
@Udit-takkar
Copy link
Author

Okay @gnprice did all the changes you suggested and cleaned the git history

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.

3 participants