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

Multi dark #20 #20

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

aditianshu
Copy link

image

Making the toggle into a select menu to enable choices between various dark themes.

@plxity
Copy link
Collaborator

plxity commented Mar 29, 2020

@aditianshu Please provide a demo link.

@aditianshu
Copy link
Author

Here's the demo.

https://codepen.io/aditianshu/pen/OJVavKd

Copy link
Collaborator

@yellowwoods12 yellowwoods12 left a comment

Choose a reason for hiding this comment

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

@aditianshu Kindly set the default theme to dark.

@yellowwoods12
Copy link
Collaborator

Also there's more that can be done for this specific enhancement that will be done in separate issues to encourage participation. After this slight change, we can merge this one for now.
@plxity what do you think?

@aditianshu
Copy link
Author

@yellowwoods12 made the required changes.

Copy link
Member

@AdityaSrivast AdityaSrivast left a comment

Choose a reason for hiding this comment

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

@aditianshu Also make appropriate changes in the docs

.vscode/settings.json Outdated Show resolved Hide resolved
src/App.css Show resolved Hide resolved
@plxity
Copy link
Collaborator

plxity commented Apr 4, 2020

Also there's more that can be done for this specific enhancement that will be done in separate issues to encourage participation. After this slight change, we can merge this one for now.
@plxity what do you think?

I think we need some more changes on this before merging. Let's provide a way to add as many themes as user wants. The idea is to not keep some fixed number of themes and provide the user with an option to add as many required.

@aditianshu I would suggest counting the number of "data-theme" present in CSS file and append their names in the dropdown. SO user can add as many themes as required. and please specify the issue number in this PR.

@yellowwoods12 We can't merge this in master branch let's make a different branch for adding this multiple options feature. What do you suggest?

@yellowwoods12
Copy link
Collaborator

yellowwoods12 commented Apr 5, 2020

I think we need some more changes on this before merging. Let's provide a way to add as many themes as user wants. The idea is to not keep some fixed number of themes and provide the user with an option to add as many required.

@plxity actually I was thinking of providing 4-5 general themes and then a custom theme wherein the RGB value of the background and the font can be entered or maybe selected from a color chart and the theme will be rendered.
But this should be done in a separate PR as it is a lot of changes for a single PR.
Yes we should merge it in a separate branch for now.

@aditianshu aditianshu changed the title Multi dark Multi dark #20 Apr 5, 2020
@aditianshu
Copy link
Author

@yellowwoods12 have made the changes mentioned by @AdityaSrivast
Do I have to do anything further?

Copy link
Collaborator

@yellowwoods12 yellowwoods12 left a comment

Choose a reason for hiding this comment

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

Looks fine to me. @plxity what do you think?

Copy link
Collaborator

@yellowwoods12 yellowwoods12 left a comment

Choose a reason for hiding this comment

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

@plxity Have a look.

@plxity
Copy link
Collaborator

plxity commented May 30, 2020

@yellowwoods12 A lot of development is still required on this. But this can be merged for further changes in a different branch. @aditianshu can you raise this PR for added-multi-branch-option branch?

@aditianshu
Copy link
Author

Raised the PR on a different branch.

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.

4 participants