Skip to content

some elements changed to dark mode#2

Merged
joshuawootonn merged 21 commits intojoshuawootonn:mainfrom
IsaacHatton:main
Dec 20, 2023
Merged

some elements changed to dark mode#2
joshuawootonn merged 21 commits intojoshuawootonn:mainfrom
IsaacHatton:main

Conversation

@IsaacHatton
Copy link
Copy Markdown
Contributor

Changing some elements to make them work with dark mode, definitely not finished in all elements as I don't have a crossway key so I can't load up the entire app.

@vercel
Copy link
Copy Markdown

vercel bot commented Dec 17, 2023

Someone is attempting to deploy a commit to a Personal Account owned by @joshuawootonn on Vercel.

@joshuawootonn first needs to authorize it.

@IsaacHatton IsaacHatton marked this pull request as draft December 17, 2023 13:11
@vercel
Copy link
Copy Markdown

vercel bot commented Dec 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
type-the-word ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 20, 2023 0:36am

@IsaacHatton
Copy link
Copy Markdown
Contributor Author

Thanks for making the vercel deploy preview work, I will try and make the remaining required changes by referencing this.

@joshuawootonn
Copy link
Copy Markdown
Owner

Hey @IsaacHatton. Thanks for putting this together! If you rebase onto main I just updated the project to load Genesis 1, Psalm 23, and James 1 without and API key. Hope that is helpful!

@IsaacHatton
Copy link
Copy Markdown
Contributor Author

Thanks, that's really helpful!
Will finish making the rest of the app functional in dark mode now.

@joshuawootonn
Copy link
Copy Markdown
Owner

Sweet!

@joshuawootonn
Copy link
Copy Markdown
Owner

Hey also, I like the idea of having this based solely on system preferences. So no need to add a toggle button!

@IsaacHatton
Copy link
Copy Markdown
Contributor Author

Hi,
I have made all the changes that I have spotted, please can you check as I may have missed one somewhere (also I haven't tested what happens when a user logs in as the menu changes and the history page because I haven't generated myself a development Google OAuth ID/Secret)

Copy link
Copy Markdown
Owner

@joshuawootonn joshuawootonn left a comment

Choose a reason for hiding this comment

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

Hey! this is looking great. I was worried that the white on black would be to strong, but I am loving it.

I have two main comments:

  • I would prefer to not add custom styles to the scroll bar and instead user color-scheme: dark
  • I would prefer to keep text color changes in tailwind typography as much as we can. You can tell where I am using tailwind typography by the prose class.

I added two commits:

  • I added styles for the history page
  • and made the svg-outline (what I am using for focus indication) compatible with dark mode

Overall though this is going great. Thanks for jumping in 🙌🏼

<Navigation />

<main className="prose relative mx-auto w-full flex-grow pt-4 text-lg lg:pt-8">
<main className="prose relative mx-auto w-full flex-grow pt-4 text-lg lg:pt-8 dark:text-white">
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This prose class indicates to @tailwindcss/typography that all of the descendants should be automatically styled. Tailwind typography is just a tailwind plugin for adding some great default styles to markup.

I think the correct solution here is: https://tailwindcss.com/docs/typography-plugin#adapting-to-dark-mode

But if that doesn't work add these styles with prose-headings:dark:text-white for example. More details.

@joshuawootonn
Copy link
Copy Markdown
Owner

because I haven't generated myself a development Google OAuth ID/Secret)

Is this just because you didn't want to login with google? Or did you run into something weird when logging in?

I am asking because the app is in the middle of verification right now.

@IsaacHatton
Copy link
Copy Markdown
Contributor Author

because I haven't generated myself a development Google OAuth ID/Secret)

Is this just because you didn't want to login with google? Or did you run into something weird when logging in?

I am asking because the app is in the middle of verification right now.

When I tried to login on the deploy preview, the url of the deploy preview didn't match the website so it refused.
When I am running it locally, I just put 0 as my environment variable for ID/Secret, as I don't have anything to put there.
When I login on the main site, it works fine.

@IsaacHatton
Copy link
Copy Markdown
Contributor Author

Hi @joshuawootonn,

I have tried to make some changes, please can you check if they are working properly.
In the arena, it still seems to need dark:text-white for some reason, I know its a mistake I am making but I can't see where it is.

Hey! I made these changes because I want to have as small of a blast radius as I can with this PR. The reason I went with tailwind typography is because you can style things nicely without having styles on every element in your markup. I would love it if you could go through each of the remaining files and double check that they actually are required.
@joshuawootonn
Copy link
Copy Markdown
Owner

@IsaacHatton I went through these files line by line to see what was needed and not.
CleanShot 2023-12-19 at 07 07 11@2x

I did this because I want as much of the dark mode styles to come from inherited colors as possible. In theory, it would be great if we could just set dark:text-white on the body element and just take care of the few exceptions to that, and just let all elements inherit that color.

The reason you were running into a problem in arena.tsx was because you had dark:invert-prose instead of dark:prose-invert. Happens to the best of us :)

If you could go through the remaining files and double check that only the critical things have changed I would appreciate it.

If you can do that I can resolve the conflict and get this merged tomorrrow.

@IsaacHatton
Copy link
Copy Markdown
Contributor Author

Ok, I will go through them shortly.

@IsaacHatton
Copy link
Copy Markdown
Contributor Author

I have gone through the files, think I have caught all of them, but I may have missed some.

@joshuawootonn
Copy link
Copy Markdown
Owner

Merged! Thanks for the help @IsaacHatton 🙌🏼

@joshuawootonn
Copy link
Copy Markdown
Owner

Yo @IsaacHatton. I have never had anyone contribute to a project I created. Just wanted to say that this is kinda big for me. I actually do really appreciate your initiative.

I would love to meet you. If you are free in January, let chat: https://calendly.com/joshuawootonn/1on1

No real expectation here, I just like meeting other developers and hearing their story.

@IsaacHatton
Copy link
Copy Markdown
Contributor Author

Hi @joshuawootonn,

I have just sent you an email with some details about me and my story.

Thanks for giving me this opportunity to contribute.

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.

2 participants