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

✨ feat: add light/dark theme switch #68

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Martimex
Copy link

@Martimex Martimex commented Feb 7, 2024

Hello!

For this feature I tried to apply as little changes to codebase as possible. In case of any doubts / questions about this PR, please let me know. Also I would really appreciate any related feedback. If this PR need some fixes before accepting, I am fine making additional changes to it.

Fixes: #44



What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Enhancement
  • Documentation Update

What I did

I have added a new button that makes toggling between canvas themes a smooth experience. The button is responsible for changing the canvas background color and text color. No other canvas appliances (like images, icons, etc.) are affected. By hovering over it, user can see a short message of what would happen after pressing the switch.

Take a look at below screenshots to see the feature in action:


Initial state (on load):

Screenshot1

Before pressing (on hover):

Screenshot2

After pressing (on hover):

Screenshot3


Original commit message:


✨ feat: add light/dark theme switch

Include a new switch button that allows users to
preview their README files for both light and
dark themes (default theme is dark one).

This feature aims to reduce the issue with black
colored icons (mostly devicons / simple icons)
being barely visible on the dark canvas. Also it
is a nice UX enhancement for those users who
simply prefer light theming.

Fixes: #44

Include a new switch button that allows users to
preview their README files for both light and
dark themes (default theme is dark one).

This feature aims to reduce the issue with black
colored icons (mostly devicons / simple icons)
being barely visible on the dark canvas. Also it
is a nice UX enhancement for those users who
simply prefer light theming.

Fixes: maurodesouza#44
Copy link

vercel bot commented Feb 7, 2024

Someone is attempting to deploy a commit to a Personal Account owned by @maurodesouza on Vercel.

@maurodesouza first needs to authorize it.

@@ -6,6 +6,7 @@ export enum Events {
CANVAS_REORDER_SECTIONS = 'canvas.section.reorder',
CANVAS_DUPLICATE_SECTION = 'canvas.section.duplicate',
CANVAS_CLEAR_SECTIONS = 'canvas.clear',
CANVAS_SWITCH_THEME = 'canvas.switchTheme',
Copy link
Owner

Choose a reason for hiding this comment

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

Change the event name to APP_SET_THEME = "app.set.theme"

Comment on lines +48 to +51

switchTheme = (theme: boolean) => {
this.emit(Events.CANVAS_SWITCH_THEME, theme);
};
Copy link
Owner

Choose a reason for hiding this comment

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

Move it to the app handle (the file need to be created)

import { themes } from "styles"
import { Modals, Events } from 'types';

import { BaseEventHandle } from './base';

class AppHandleEvents extends BaseEventHandle {
  constructor() {
    super();

    this.close = this.close.bind(this);
  }

  theme(theme: keyof typeof themes) {
    this.emit(Events.APP_SET_THEME, theme);
  }
}

export { ModalHandleEvents };

Then, include it in the index handles

  app = new AppHandleEvents();

@@ -24,7 +26,7 @@ import { CanvasErrorFallback } from './error';

const Canvas = () => {
const { extensions } = useExtensions();
const { sections, currentSection, previewMode } = useCanvas();
const { sections, currentSection, previewMode, lightTheme } = useCanvas();
Copy link
Owner

Choose a reason for hiding this comment

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

Here, that logic with be moved to the pages/_app.tsx

const { theme } = useTheme();

const curTheme = themes[theme]

return (
  <ThemeProvider theme={curTheme }>
  ...
)

@@ -41,9 +43,26 @@ const Canvas = () => {
<S.Container
onContextMenu={handleOpenContextMenu}
fullHeight={hasError || !hasSection}
isLightTheme={lightTheme}
Copy link
Owner

Choose a reason for hiding this comment

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

It can be removed

Comment on lines +48 to +62
<S.Wrapper isLeftAligned={false}>
<Tooltip
position="right"
content={`Preview: ${lightTheme ? 'dark' : 'light'} mode`}
variant="info"
>
<S.Button
aria-label={`Preview: ${lightTheme ? 'dark' : 'light'} mode`}
onClick={() => events.canvas.switchTheme(lightTheme)}
variant="success"
>
{lightTheme ? <MoonIcon size={16} /> : <SunIcon size={16} />}
</S.Button>
</Tooltip>
</S.Wrapper>
Copy link
Owner

Choose a reason for hiding this comment

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

The theme switch can be done together with the other actions.

image

And keep it on the template preview.

image

{hasSection && !previewMode && (
<S.Wrapper>
<S.Wrapper isLeftAligned={true}>
Copy link
Owner

Choose a reason for hiding this comment

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

It can be removed

@@ -61,7 +80,7 @@ const Canvas = () => {
onChange={setHasError}
>
{previewMode && (
<S.Wrapper>
<S.Wrapper isLeftAligned={true}>
Copy link
Owner

Choose a reason for hiding this comment

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

It can be removed

Copy link
Owner

Choose a reason for hiding this comment

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

The theme switch logic needs to be separated into another react context, which can be called ThemeContext

Copy link
Owner

Choose a reason for hiding this comment

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

All changes in this file can be reverted because we will use the theme feature of the styled-components to make the theme change.

ref: https://styled-components.com/docs/advanced#theming

@maurodesouza
Copy link
Owner

Hi @Martimex

I'm delighted with your contribution! Thanks so much!

Regarding the PR content, I believe switching to a global theme would be more advantageous.

I've requested changes. Please let me know if you can implement them or if you need any more information.

Summary:

  • add a new react context/event handle to manage theme changing;
  • let the switch button together with other actions;
  • use the styled-components features to change the theme;
  • create a light theme file within the styles/theme directory (by duplicating default theme values and modifying only the colors), and export a mapping from there.
// styles/themes/index.ts

import { defaultTheme } from './default';
import { lightTheme } from './light';

const themes = {
  dark: defaultTheme,
  light: lightTheme 
}

export { theme }

@Martimex
Copy link
Author

Thank you so much for the detailed insight in this PR !


I've requested changes. Please let me know if you can implement them or if you need any more information.

I am definitely going to apply requested changes to the branch and add an updated PR soon, probably within few days time.

@Martimex
Copy link
Author

Hey @maurodesouza,

I have been trying to implement the changes. So far, the theme change functionality is completed and it works nicely within the whole app.


The theme change functionality is implemented by using APP_SET_THEME synthetic event, that finally leads to this function (in _app.tsx) :

  const [currTheme, setCurrTheme] = useState(themes['dark']);

  const handleSetTheme = (e: CustomEvent) => {
    setCurrTheme(themes[e.detail]);
  }

The currTheme variable is then provided as a < ThemeProvider > argument, which causes rerender whenever theme changes (also inside _app.tsx) :

<ThemeProvider theme={currTheme}>

There is one difficult issue, though. Whenever I try to change the theme, this happens :

issue.mp4

After some investigation, I found out that the elements with this weird behaviour are ones, which use any transition from Framer Motion. By that I mean those two Reorder Lists visible on the video above.

Then, after some trials I came up with only this solution, which minifies (but does not get rid off) the problem :

solution

Left part of the image is code from _app.tsx ; the right part is global.ts (where the global styles are stored).

So, after adding the code from the image :

current.mp4

the problem is not present if user clicks the button with a brief delay. However, as soon as button is being clicked consecutively around 3 times, then the reflow issue appears again.

Honestly, I think I am running out of ideas on how to deal with this reflow thing properly. Current approach is more like a workaround IMO. Maybe there is an option to trigger theme change in another way, but it's just me cannot figure it out.


Should we try to find a different way to implement the feature then ? Or maybe you have had experienced similiar problem before and you managed to solve it ? Please let me know, I would appreciate any response from you.

@maurodesouza
Copy link
Owner

Hi @Martimex! Apologies for my lateness!

Could you commit/send me what you have? I can take it from there and finish it up.

You've already done a great job! Thanks a lot

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.

[FEATURE]: light/dark theme switch
3 participants