Skip to content

Conversation

@kailash360
Copy link
Contributor

@aaryak-shah, I have added different color themes for the editor and saved the Name-keyword pair in constants file. Although, we need to import each of the themes manually in the index.js of homepage folder. The theme is switched using a select element which updates the currentTheme state.

Cobalt is set as the default theme

This is for Issue #7 .

Copy link
Owner

@nafees87n nafees87n left a comment

Choose a reason for hiding this comment

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

@kailash360 Please check. I have added some review comments.
Please refactor the code and put all the state variables together at the top of the component.
Also put all the useEffects together. Currently, the useEffect for checking screen size is hanging at the bottom.

}

//State to change the themes
const [currentTheme, setCurrentTheme] = useState("cobalt")
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should use useLocalStorage here so that the theme persists with the user data

onChange={(e)=>{setCurrentTheme(e.target.value)}}
style={{ display: visible ? 'block' : 'none' }}
>
<option value="cobalt" selected>Default</option>
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
<option value="cobalt" selected>Default</option>
<option value="cobalt" selected>Default Theme</option>

Can we make it Default Theme so that the user know this selector would change themes?

@nafees87n nafees87n linked an issue Oct 7, 2021 that may be closed by this pull request
@kailash360
Copy link
Contributor Author

kailash360 commented Oct 7, 2021

@nafees87n, I have made the suggested changes. Please have a look. I have also moved the value of default theme to the constants,js file, so that it can be changed later on easily.

Copy link
Owner

@nafees87n nafees87n left a comment

Choose a reason for hiding this comment

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

@kailash360
Have a look at the suggestion and understand the working of useLocalStorage which is a custom hook.

@kailash360
Copy link
Contributor Author

@nafees87n, I have made the required changes, please take a look

refactored to put cached state variables at top
@nafees87n
Copy link
Owner

Thank You @kailash360. Looks good to merge now.
I hope you have learned something new while implementing this feature.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Develop a theme switcher for the code editor UI

2 participants