Resolve "Dark mode toggle button" - [merged] #58

Closed thisismydesign closed 4 years ago

In GitLab by @nkapolcs on Jul 10, 2019, 20:51

Merges 23-dark-mode-toggle-button -> master

Issue #23

Migrate to next.js the ToggleButton from Zsolt page

Changes:

[x] overview next.js and config files (eslint, prettier)
[x] make some file structure change
[x] add dark mode toggle button
[x] style header to make nicer
[x] add icons to the button

captured

In GitLab by @gomorizsolt on Jul 10, 2019, 21:11

Couldn't do a thorough review, but here are some remarks.

Please create a smaller PR next time. :)
There's a requirement to add the usual sun/moon icons to the toggle button.
Do not deal with tests.
It's unnecessary to add the GitLab label to the PR.

In GitLab by @nkapolcs on Jul 10, 2019, 21:51

changed the description

In GitLab by @gomorizsolt on Jul 11, 2019, 09:19

Let me suggest a PR description structure. This is just my personal preference, but you may find some parts of it useful.

Branch naming:

https://github.com/c-hive/guides/blob/master/universal/code-style.md#branches

Issue #<issue_number>.

TODOs - not sure if it's needed.

<brief_description> - there're some cases when a brief description helps to understand the solution, but sometimes this can be unnecessary.

Changes:
- <change_1>
- <change_2>

<screenshot>, <attachments> - useful when there're some UI changes so the reviewer doesn't have to test it locally.

In GitLab by @nkapolcs on Jul 11, 2019, 09:33

added 1 commit

83dfb165 - Add icons to toggle button related to #23

Compare with previous version

In GitLab by @nkapolcs on Jul 11, 2019, 09:34

marked the task add icons to the button as completed

In GitLab by @nkapolcs on Jul 11, 2019, 09:46

changed the description

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

plugins: ["react"],

Please see our js boilerplate:

https://github.com/c-hive/js-boilerplate/blob/master/.eslintrc.js

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

We don't need these resolvers. This is just a convenient feature so that avoid going up to the root directory all the time. Furthermore, it can't be used without Webpack. :)

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

Let's remove this hook as we aren't focusing on tests now.

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

We have already released the NPM version of this package:

https://github.com/c-hive/team-contribution-calendar#npm

I'm not sure how it works in practice. Could you give it a try, please?

It'd be nice to avoid using the CDN link here.

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

export const gitHubUsernames = ["gomorizsolt", "thisismydesign", "bencefrelli", "nkapolcs"];

Please follow our ReactJS naming conventions:

https://github.com/c-hive/guides/blob/master/react/file-structure-naming.md#casing

Also, the current team is as follows:

export const gitHubUsernames = ["gomorizsolt", "thisismydesign", "megant", "nkapolcs"];

Please update the other array accordingly.

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

I'm not sure if this font is imported as an external URL.

I'd rather stick with the body styles and remove the remaining ones - i.e. the a and p.

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

This wrapper is superfluous.

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

IMO it's better if we remove all the test related files from the repository.

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

Let's display a fancy loading icon instead of the raw text. :)

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

Please see our styling guide:

https://github.com/c-hive/guides/blob/master/react/styling-and-css.md#naming

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

This comes from my repository. We use comments in special cases:

https://github.com/c-hive/guides/blob/master/universal/code-style.md#code-style

Please do not just copy-paste the lines. It's likely to happen that you'll find some parts that can be refactored as it was a few months ago when I created the first version of my site. :)

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

export const useDarkMode = () => {

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

Do we really need this component?

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

IMO this GetTheme function is unnecessary - and it doesn't really follow our naming conventions. :)

See https://gitlab.com/c-hive/c-hive.dev/merge_requests/24#note_190662901.

In GitLab by @gomorizsolt on Jul 11, 2019, 10:30

Let's store these theme styles separately as a resource file, so we can get rid of this function.

In GitLab by @gomorizsolt on Jul 11, 2019, 10:34

The design looks good, but please try to keep an eye on our conventions. Left another important note in https://gitlab.com/c-hive/c-hive.dev/merge_requests/24#note_190662887.

In GitLab by @thisismydesign on Jul 11, 2019, 12:02

Let's do TCC related changes separately.

In GitLab by @thisismydesign on Jul 11, 2019, 12:03

Good point, if I remember correctly you already made a loading hook?

In GitLab by @gomorizsolt on Jul 11, 2019, 12:08

Perhaps, but couldn't find it. However, I think that's not needed here.

In GitLab by @thisismydesign on Jul 11, 2019, 12:16

Good comments from @gomorizsolt.

One addition from my side: turns out toggles are less intuitive than I thought. See this SO thread: https://ux.stackexchange.com/questions/1318/should-a-toggle-button-show-its-current-state-or-the-state-to-which-it-will-chan

I propose we display states on the side and highlight the selected one them as pointed out in this answer. E.g. (but with out icons of course):

In GitLab by @thisismydesign on Jul 11, 2019, 12:19

Perhaps this could be implemented in a reusable way. The function, hook, etc should take two pictures and apply opacity to the unselected one.

In GitLab by @nkapolcs on Jul 11, 2019, 21:17

changed this line in version 3 of the diff

In GitLab by @nkapolcs on Jul 11, 2019, 21:17

added 1 commit

5f920497 - Apply suggestion to .eslintrc.js

Compare with previous version

In GitLab by @nkapolcs on Jul 11, 2019, 22:39

changed this line in version 4 of the diff

In GitLab by @nkapolcs on Jul 11, 2019, 22:39

changed this line in version 4 of the diff

In GitLab by @nkapolcs on Jul 11, 2019, 22:39

changed this line in version 4 of the diff

In GitLab by @nkapolcs on Jul 11, 2019, 22:39

changed this line in version 4 of the diff

In GitLab by @nkapolcs on Jul 11, 2019, 22:39

changed this line in version 4 of the diff

In GitLab by @nkapolcs on Jul 11, 2019, 22:39

changed this line in version 4 of the diff

In GitLab by @nkapolcs on Jul 11, 2019, 22:39

changed this line in version 4 of the diff

In GitLab by @nkapolcs on Jul 11, 2019, 22:39

changed this line in version 4 of the diff

In GitLab by @nkapolcs on Jul 11, 2019, 22:39

changed this line in version 4 of the diff

In GitLab by @nkapolcs on Jul 11, 2019, 22:39

added 4 commits

2904b0a2 - updated eslint config related to #23
4165117c - updated package config related to #23
fde0f2b0 - updated eslint config related to #23
467f699e - add preloader related to #23

Compare with previous version

In GitLab by @nkapolcs on Jul 11, 2019, 22:50

Thx :) You're right, as we're not focusing on tests now.

In GitLab by @nkapolcs on Jul 11, 2019, 22:58

Should I create for this an another issue?

In GitLab by @nkapolcs on Jul 11, 2019, 23:11

I create that

commit: 467f699e

file: https://gitlab.com/c-hive/c-hive.dev/blob/23-dark-mode-toggle-button/src/components/GlobalTheme/GlobalTheme.js#L12

I think this will be a good place for the css in this case, but let me know.

In GitLab by @nkapolcs on Jul 11, 2019, 23:12

added 1 commit

3cc6ad24 - delete unnecessary style related to #23

Compare with previous version

In GitLab by @nkapolcs on Jul 11, 2019, 23:16

I think this will be good for multiple pages as a layout wrapper (header, footer, layout etc).

In GitLab by @nkapolcs on Jul 11, 2019, 23:22

changed this line in version 6 of the diff

In GitLab by @nkapolcs on Jul 11, 2019, 23:22

added 1 commit

a96192f4 - small fixes related to #23

Compare with previous version

In GitLab by @nkapolcs on Jul 11, 2019, 23:25

Thanks for the detailed review it helps a lot @gomorizsolt I will finish the rest tomorrow.

In GitLab by @thisismydesign on Jul 12, 2019, 00:00

Should be covered in https://gitlab.com/c-hive/c-hive.dev/issues/24, let's clarify the details as we get there.

In GitLab by @gomorizsolt on Jul 12, 2019, 10:08

Alright, resolved.

In GitLab by @gomorizsolt on Jul 12, 2019, 10:16

layout wrapper (header, footer, layout etc).

A layout wrapper for the layout component? :) Joking.

How about naming it differently? E.g. PageWrapper? Feel free to come up with something else.

I'd say that we can restructure the folders, but let's leave it as is. We can refine these minor things along the way.

c-hive / dotdev

Resolve "Dark mode toggle button" - [merged] #58