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

feat(ui): implement dark mode #133

Merged
merged 53 commits into from
Nov 25, 2023
Merged

feat(ui): implement dark mode #133

merged 53 commits into from
Nov 25, 2023

Conversation

kreshnnikgashi
Copy link
Member

@kreshnnikgashi kreshnnikgashi commented Nov 7, 2023 •
edited

This closes #116 and #123

@kreshnnikgashi kreshnnikgashi linked an issue Nov 7, 2023 that may be closed by this pull request
Implement dark mode #116
Closed
@kreshnnikgashi
feat(ui): implement dark mode
99a2b0d
@kreshnnikgashi
feat(ui): implement dark mode
81f8f6e
@kreshnnikgashi
feat(ui): implement dark mode
f9742b5
@kreshnnikgashi
fix(ui): remove unnecessary vars
c90ff93
@kreshnnikgashi
feat(ui): add theme_type as const
76276d8
@kreshnnikgashi
feat(ui): add colors for light mode
e8f8f92
@kreshnnikgashi
chore(build): add tailwind-merge and clsx
0fef648
@kreshnnikgashi
feat(ui): add cn function to conditionally render classess with tailw…
4a82ce1 
…ind-merge and clsx
@kreshnnikgashi
feat(ui): add theme_type
625374c
@kreshnnikgashi
refactor(ui/toggle): conditionally render classnames with cn function
c17a45e
@kreshnnikgashi
feat(ui): rename colors, change background when toggle dark mode
ed91b3c
@kreshnnikgashi
refactor(ui): make svg reusable
2657f67
@kreshnnikgashi
feat(ui): add icons
c426e92
@kreshnnikgashi
chore(build): pnpm-lock
60e4ad6
@kreshnnikgashi
fix(ui/build): remove classnames dependency
169d1fb
@kreshnnikgashi
fix(ui): revert back old colors
ec60886
@kreshnnikgashi
feat(ui): add twMerge into icons
6f5baf6
@kreshnnikgashi
lint(ui): disable react/display-name
c38db84
@kreshnnikgashi
test(ui): add unit testing for useThemeStore
8b7b36f
@kreshnnikgashi
test(ui): add unit tests for useTheme hook
d2c683e
@kreshnnikgashi
test(ui): update zustand mocks
ac4bd34
@kreshnnikgashi
fix(ui): remove context for dark mode
ac862f0
@kreshnnikgashi
test(ui): add test when user clicks toggle it should successfully cha…
5b2df78 
…nge theme color
@kreshnnikgashi
test(ui): add test case for themestore
73aae13
@danstarns
Copy link
Member

This is looking great, some initial comments, and expect more to come.

Npm Icon
I find this jarring, it seems to be off to one side.

@kreshnnikgashi I think we should just remove the npm icon now and direct only to the GitHub - wdyt?

Screenshot 2023-11-10 at 12 01 53

Config Icon
Using the user icon could be confusing to developers as we also have a user for the login, Let's keep anything user-related to the cloud and keep this as a cog only.

Screenshot 2023-11-10 at 11 55 49

Editor Color
I think the purple in the dark background is hard to see, try something a little bit lighter.

Screenshot 2023-11-10 at 11 56 41

@danstarns
Copy link
Member

I see we are having issues with the modal colors, I think we should change the modal background to be the same as the trace views, so that we dont have to invert the pilll or background colors, wdyt? @kreshnnikgashi

Screenshot 2023-11-10 at 12 04 24

danstarns
danstarns reviewed Nov 10, 2023
View reviewed changes
apps/ui/src/__mocks__/matchMedia.ts
matches: false,
media: query,
onchange: null,
addListener: jest.fn(), // Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using deprecated, methods, can we change to the latest?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this is the latest: https://jestjs.io/docs/manual-mocks#mocking-methods-which-are-not-implemented-in-jsdom - however I'm gonna remove these 2 deprecated methods

@kreshnnikgashi
Copy link
Member Author

kreshnnikgashi commented Nov 10, 2023 •
edited

I see we are having issues with the modal colors, I think we should change the modal background to be the same as the trace views, so that we dont have to invert the pilll or background colors, wdyt? @kreshnnikgashi

Screenshot 2023-11-10 at 12 04 24

I agree @danstarns - should this be the case for all modals or just for trace viewer ?

@kreshnnikgashi
feat(ui): implement dark mode on modal
d7ad1ea
@kreshnnikgashi
feat(ui): refactor modal
b23ad83
@kreshnnikgashi
refactor(ui): remove old modal
5e28168
@kreshnnikgashi
test(ui/modal): add 2 test cases for modal
81db3b4
@kreshnnikgashi
chore(build): add framer-motion
bc95c15
@kreshnnikgashi
refactor(ui): reduce size of traces
e48981e
@kreshnnikgashi
feat(ui): add icons for dark mode
a0cae8f
@kreshnnikgashi
feat(ui): add modal
2e78db9
@kreshnnikgashi
refactor(ui): modal
42715e0
@kreshnnikgashi
feat(ui): modal
0e08e1e
@kreshnnikgashi
Merge branch 'main' of https://github.com/rocket-connect/graphql-debu…
5096c00 
…gger into feat/dark-mode
@kreshnnikgashi
test(ui): add window.MatchMedia support on jest
75c9f65
@kreshnnikgashi
test(ui): add unit testing for modal
0816504
@kreshnnikgashi
chore(build): sync pnpm with ui/package.json
164e2ba
@kreshnnikgashi
fix(ui): remove unused vars
550c1c5
@kreshnnikgashi
refactor(ui): rename colors, use the same colors in all over the project
ff3c11d
@kreshnnikgashi
refactor(ui): change color names
eac2744
@kreshnnikgashi
fix(e2e): colors
502b069
@kreshnnikgashi
test(e2e): add back ids to expand button and full trace viewer modal
55f8b8c
@kreshnnikgashi
fix(ui): remove deprecated methods from matchMedia
08448c1
@kreshnnikgashi
feat(ui): add new colors
6848df9
@kreshnnikgashi
test(ui): revert back deprecated methods, as tests are failing
bb83523
danstarns
danstarns approved these changes Nov 24, 2023
View reviewed changes
Copy link
Member

@danstarns danstarns left a comment

Choose a reason for hiding this comment

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

It looks like it's the flaky color e2e tests blocking this. Let's skip those lines and make a separate issue linking to each line.

Great work @kreshnnikgashi let's merge and 🚢 it.

@kreshnnikgashi kreshnnikgashi merged commit 59f0082 into main Nov 25, 2023
13 checks passed
@kreshnnikgashi kreshnnikgashi deleted the feat/dark-mode branch November 25, 2023 08:33
@danstarns danstarns mentioned this pull request Nov 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danstarns danstarns danstarns approved these changes

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

Successfully merging this pull request may close these issues.

Implement dark mode
2 participants
@kreshnnikgashi @danstarns