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

Added: Light Theme Mode #63

Conversation

GaganpreetKaurKalsi
Copy link
Collaborator

#44 : Successfully added Light Mode to caucus

I have added classes such as tone1, tone2, tone3 to the components. In future, if you would like to add other themes to the components, you just need to specify it in the Theme.style.css file. Below are some screenshots for your reference.
image
image
image
image
image

Kindly review and let me know if I need to do something on my part.
Thanks!

@Rishabh-malhotraa
Copy link
Owner

@all-contributors please add @GaganpreetKaurKalsi for code and design

@allcontributors
Copy link
Contributor

@Rishabh-malhotraa

I've put up a pull request to add @GaganpreetKaurKalsi! 🎉

@Rishabh-malhotraa
Copy link
Owner

I am not a big fan of how CSS modules way of switching themes. I am tempted to switch to styled-components for all things CSS, but that would be a big restructure. What do you think?

@GaganpreetKaurKalsi
Copy link
Collaborator Author

I am not a big fan of how CSS modules way of switching themes. I am tempted to switch to styled-components for all things CSS, but that would be a big restructure. What do you think?

Definitely it would be a big restructure. The problem is that many classes are not defined by the project builder. They are inbuilt classes from modules.
If you would like to switch to styled-components, no problem. It will also help removing the inline styles and then one doesn't have to quote "!important" everywhere.
I am also not a big fan of normal css, especially when working with react/typescript.

src/component/Editor/CodeMirrorEditor.tsx Outdated Show resolved Hide resolved
Comment on lines 11 to 25
<svg
aria-hidden="true"
focusable="false"
data-prefix="fas"
data-icon="expand"
className="svg-inline--fa fa-expand fa-w-14 MuiSvgIcon-root"
role="img"
xmlns="http://www.w3.org/2000/svg"
viewBox="0 0 448 512"
>
<path
fill="currentColor"
d="M0 180V56c0-13.3 10.7-24 24-24h124c6.6 0 12 5.4 12 12v40c0 6.6-5.4 12-12 12H64v84c0 6.6-5.4 12-12 12H12c-6.6 0-12-5.4-12-12zM288 44v40c0 6.6 5.4 12 12 12h84v84c0 6.6 5.4 12 12 12h40c6.6 0 12-5.4 12-12V56c0-13.3-10.7-24-24-24H300c-6.6 0-12 5.4-12 12zm148 276h-40c-6.6 0-12 5.4-12 12v84h-84c-6.6 0-12 5.4-12 12v40c0 6.6 5.4 12 12 12h124c13.3 0 24-10.7 24-24V332c0-6.6-5.4-12-12-12zM160 468v-40c0-6.6-5.4-12-12-12H64v-84c0-6.6-5.4-12-12-12H12c-6.6 0-12 5.4-12 12v124c0 13.3 10.7 24 24 24h124c6.6 0 12-5.4 12-12z"
></path>
</svg>
Copy link
Owner

Choose a reason for hiding this comment

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

Put this as svg file in assets and export the SVG here
ex: import GoogleSVG from "assets/google.svg"; More Context

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not using the SVG in image tag as I am unable to update the color upon theme change. I have made an icon file in the assets folder which contains svg returned from function.


const EditorToolbar: FC<Props> = ({ screenSize, setScreenSize }) => {
const { language, handleLanguageChange } = useContext(SettingContext) as SettingsContextType;
const { id } = useParams<Record<string, string>>();
Copy link
Owner

Choose a reason for hiding this comment

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

use roomID() hook to get id

Comment on lines 34 to 35
const [screenSize, setScreenSize] = useState("normal");
const [theme, setTheme] = useState("dark");
Copy link
Owner

Choose a reason for hiding this comment

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

It would be better to store this information in the global context in service/SettingContext.tsx

we already have a variable called theme for setting editor-theme; let's rename that to editorTheme and define theme variable for global light or dark mode

@Rishabh-malhotraa
Copy link
Owner

Rishabh-malhotraa commented Oct 8, 2021

I am not a big fan of how CSS modules way of switching themes. I am tempted to switch to styled-components for all things CSS, but that would be a big restructure. What do you think?

Definitely it would be a big restructure. The problem is that many classes are not defined by the project builder. They are inbuilt classes from modules. If you would like to switch to styled-components, no problem. It will also help removing the inline styles and then one doesn't have to quote "!important" everywhere. I am also not a big fan of normal css, especially when working with react/typescript.

Let me do some research about the pros and cons of styled-components, because CSS modules are really annoying when you require to write some complex CSS.
@GaganpreetKaurKalsi Have you used any CSS in JS solution before like JSS, emotion or styled components?

@GaganpreetKaurKalsi
Copy link
Collaborator Author

I am not a big fan of how CSS modules way of switching themes. I am tempted to switch to styled-components for all things CSS, but that would be a big restructure. What do you think?

Definitely it would be a big restructure. The problem is that many classes are not defined by the project builder. They are inbuilt classes from modules. If you would like to switch to styled-components, no problem. It will also help removing the inline styles and then one doesn't have to quote "!important" everywhere. I am also not a big fan of normal css, especially when working with react/typescript.

Let me do some research about the pros and cons of styled-components, because CSS modules are really annoying when you require to write some complex CSS. @GaganpreetKaurKalsi Have you used any CSS in JS solution before like JSS, emotion or styled components?

I have worked with styled-components.

@GaganpreetKaurKalsi
Copy link
Collaborator Author

I have made all the requested changes. You can have a look now!

@Rishabh-malhotraa
Copy link
Owner

@GaganpreetKaurKalsi After doing some research about how to implement different themes, I think CSS modules would work well too, we just need to do some workaround.

  • Since MUI injects the CSS at the end of the HTML it takes precedence over all the CSS we write in our css.modules file and we have to write !important everywhere, we can fix this by using
<StyledEngineProvider injectFirst>
  {/* Your component tree. Now you can override MUI's styles. */}
</StyledEngineProvider>

MUI Docs Explaining the same

If we do this we can also remove all the CSS-in-JS we have used at different places, like we do here

  • the way I want to implement different themes is by using a JSON object and change the CSS-variable values using JS
const lightTheme = {
  "--primary": "#31e981",
  "--seconday": "#000000",
  };

//This is our dark mode
const darkTheme = {
  "--primary": "#286843",
  "--seconday": "#ffffff",
};
export { lightTheme, darkTheme };
const ApplyTheme = ({ theme, children }) => {
  //Update the CSS Variables
  const updateCSSVariables = theme => {
    const arrayOfVariableKeys = Object.keys(theme);
    const arrayOfVariableValues = Object.values(theme);

    //Loop through each array key and set the CSS Variables
    arrayOfVariableKeys.forEach((cssVariableKey, index) => {
      //Based on our snippet from MDN
      document.documentElement.style.setProperty(
        cssVariableKey,
        arrayOfVariableValues[index]
      );
    });
  };

Here is an article which explains theming in details

@GaganpreetKaurKalsi
Copy link
Collaborator Author

@GaganpreetKaurKalsi After doing some research about how to implement different themes, I think CSS modules would work well too, we just need to do some workaround.

  • Since MUI injects the CSS at the end of the HTML it takes precedence over all the CSS we write in our css.modules file and we have to write !important everywhere, we can fix this by using
<StyledEngineProvider injectFirst>
  {/* Your component tree. Now you can override MUI's styles. */}
</StyledEngineProvider>

MUI Docs Explaining the same

If we do this we can also remove all the CSS-in-JS we have used at different places, like we do here

  • the way I want to implement different themes is by using a JSON object and change the CSS-variable values using JS
const lightTheme = {
  "--primary": "#31e981",
  "--seconday": "#000000",
  };

//This is our dark mode
const darkTheme = {
  "--primary": "#286843",
  "--seconday": "#ffffff",
};
export { lightTheme, darkTheme };
const ApplyTheme = ({ theme, children }) => {
  //Update the CSS Variables
  const updateCSSVariables = theme => {
    const arrayOfVariableKeys = Object.keys(theme);
    const arrayOfVariableValues = Object.values(theme);

    //Loop through each array key and set the CSS Variables
    arrayOfVariableKeys.forEach((cssVariableKey, index) => {
      //Based on our snippet from MDN
      document.documentElement.style.setProperty(
        cssVariableKey,
        arrayOfVariableValues[index]
      );
    });
  };

Here is an article which explains theming in details

Okay! I got it. But this workaround is quite huge. I would suggest you merge this pull request first and open up another issue and assign that to me. I will work on it then. Also it would be great if you could merge the others as well, as I am participating in Hacktoberfest for the first time and it means a lot to me.
Thanks!

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.

2 participants