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

Implement darkmode on Learn/Platform Tools page #1896

Closed
wants to merge 19 commits into from

Conversation

oyenuga17
Copy link
Contributor

@SaySayo @punchagan please help review

@sabine
Copy link
Collaborator

sabine commented Jan 3, 2024

Hey @oyenuga17, there's a lot of conflicts here, I suppose you didn't want to include all those commits in the PR.

The only relevant one seems to be 1f2093c.

The way to tidy this up:

make a copy of your branch, then go back to this PR branch. git rebase -i main (after updating main to be the same as ocaml/ocaml.org main branch) and remove all the unnecessary commits.

@@ -88,14 +88,14 @@ let sidebar
<%s t %>
</span>
in
<a class="border-l-2 py-2 px-3 rounded-sm leading-6 <%s if current then {|font-bold text-primary bg-legacy-primary-100 border-primary|} else {|text-legacy-default hover:bg-gray-100 border-transparent|} %>" href="<%s href %>">
<a class="border-l-2 py-2 px-3 rounded-sm leading-6 font-normal <%s if current then {|text-title dark:text-dark-title bg-primary_nav_block_hover_10 dark:bg-primary_nav_block_hover_10 border-primary dark:border-dark-primary|} else {|text-content dark:text-dark-content hover:text-primary dark:hover:text-dark-primary border-transparent|} %>" href="<%s href %>">
Copy link
Contributor

Choose a reason for hiding this comment

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

It has the same background for light and dark mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @SaySayo, they both have the same color according to the design

@@ -353,7 +353,7 @@ Layout.render
<div class="flex flex-col lg:flex-row justify-between lg:space-x-20 space-y-10 lg:space-y-0">
<div class="shadow-lg p-10 bg-legacy-default dark:bg-legacy-dark-default rounded-xl lg:mb-0">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div class="shadow-lg p-10 bg-legacy-default dark:bg-legacy-dark-default rounded-xl lg:mb-0">
<div class="shadow-lg p-10 bg-white dark:bg-dark-background rounded-xl lg:mb-0">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SaySayo I think I added this changes by mistake. I'll discard the changes I made to this file for now and create a separate pr for this page when there's a design for it.

@@ -156,7 +156,7 @@ Layout.render
</div>
<div class="text-legacy-default font-bold">Powerful Type Safety Made Simple</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div class="text-legacy-default font-bold">Powerful Type Safety Made Simple</div>
<div class="text-content dark:text-dark-content font-bold">Powerful Type Safety Made Simple</div>

@@ -370,7 +370,7 @@ Layout.render
</div>
<div class="shadow-lg p-10 bg-legacy-default dark:bg-legacy-dark-default rounded-xl">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div class="shadow-lg p-10 bg-legacy-default dark:bg-legacy-dark-default rounded-xl">
<div class="shadow-lg p-10 bg-white dark:bg-dark-background rounded-xl">

@@ -1,4 +1,4 @@
let render
let render
Copy link
Contributor

Choose a reason for hiding this comment

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

Take out unnecessary white space

@@ -182,7 +182,7 @@ Layout.render
<h3 class="text-legacy-default font-bold">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<h3 class="text-legacy-default font-bold">
<h3 class="text-content dark:text-dark-content font-bold">

@SaySayo
Copy link
Contributor

SaySayo commented Jan 9, 2024

Check out the suggested changes @oyenuga17

@oyenuga17
Copy link
Contributor Author

closing this for now, will re-open when #1913 is merged

@oyenuga17 oyenuga17 closed this Jan 10, 2024
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.

None yet

4 participants