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.

Sign up for GitHub

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

Jump to bottom

Introducing Trending Dark Mode #3

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

Introducing Trending Dark Mode #3

wants to merge 15 commits into from

Conversation

saif71
Copy link

@saif71 saif71 commented Apr 1, 2019

  1. Dark Mode Added
  2. Updated Readme.md
  3. Added Informative badges

@mugli
Copy link
Member

mugli commented Apr 2, 2019

Could you provide a screenshot for the visual changes?

@mugli mugli requested a review from torifat April 2, 2019 06:18
@saif71
Copy link
Author

saif71 commented Apr 2, 2019

Could you provide a screenshot for the visual changes?

Sure. Here they are.
screenshots

also for demonstration purpose I've deployed it on :
https://avro.netlify.com/

@saif71
Copy link
Author

saif71 commented Apr 2, 2019

Dear concern,
I've also created a browser extension for chrome. Please let me know your thoughts.
https://github.com/saif71/avro-for-chromium

@sarim
Copy link
Member

sarim commented Apr 2, 2019

few technical problems.

  1. app manifest producing a 404 error and/or not working properly in your deployment.
  2. Loader (loading for the last time) related stylesheet needs to remain in index.html. If the browsers needs to wait for another stylesheet to download, it kinda defeats the purpose of loader doesn't it?
  3. There is probably room for improvement in the code for swapping themes. Right now it glitches to no stylesheet for a brief second before the theme is loaded.

@saif71
Copy link
Author

saif71 commented Apr 2, 2019

few technical problems.

  1. app manifest producing a 404 error and/or not working properly in your deployment.
  2. Loader (loading for the last time) related stylesheet needs to remain in index.html. If the browsers needs to wait for another stylesheet to download, it kinda defeats the purpose of loader doesn't it?
  3. There is probably room for improvement in the code for swapping themes. Right now it glitches to no stylesheet for a brief second before the theme is loaded.

those issues has been resolved . Please check. :)

@mugli mugli requested a review from sarim April 3, 2019 07:04
@mugli
Copy link
Member

mugli commented Apr 3, 2019

Ping @sarim. Will merge and deploy after getting lgtm from you.

@sarim
Copy link
Member

sarim commented Apr 7, 2019

Doesn't work after building - gulp build. Build process concentrates all css files into one, and your theme changing js doesn't work after that. Don't do theme switch using javascript based stylesheet change, rather include both themes in css under a prefix/class name. toggle the classname on body.
ex:

<body class="theme-dark"

@sarim
Copy link
Member

sarim commented Apr 7, 2019

@saif71
Copy link
Author

saif71 commented Apr 7, 2019

Ok. I'm checking the issues .

@saif71
Copy link
Author

saif71 commented Apr 7, 2019 •
edited
Loading

Doesn't work after building - gulp build. Build process concentrates all css files into one, and your theme changing js doesn't work after that. Don't do theme switch using javascript based stylesheet change, rather include both themes in css under a prefix/class name. toggle the classname on body.
ex:

<body class="theme-dark"

@sarim
Issues has been resolved. Please check the latest commit.

@saif71
Copy link
Author

saif71 commented May 22, 2019

vai,
did you get the chance to review the latest codes ? :)

@khaled-0
Copy link

@sarim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@torifat torifat Awaiting requested review from torifat

@sarim sarim Awaiting requested review from sarim

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@saif71 @mugli @sarim @khaled-0