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

Fix MUI theme in Jupyter AI Settings #1210

Merged
merged 2 commits into from
Jan 22, 2025
Merged

Conversation

MUFFANUJ
Copy link
Contributor

@MUFFANUJ MUFFANUJ commented Jan 21, 2025

fixes - #1175

This PR addresses the issue where the Jupyter AI settings panel does not properly utilize the JupyterLab theme provider, resulting in incorrect styling of components.

Changes:
Integrated the JupyterLab theme provider to ensure components follow the JupyterLab theme styles.
Corrected rendering issues in the dark theme to ensure proper visibility and contrast of components.
This update ensures that the settings panel is visually consistent with the rest of the JupyterLab interface in both light and dark themes.

Here is the video demo of the changes made:-
https://github.com/user-attachments/assets/135a317c-160f-4a05-b76d-61fe1d281717

@MUFFANUJ MUFFANUJ changed the title Fix: Integrate JupyterLab Theme Provider for Consistent UI Styling Fix: Jupyter AI's broken settings theme Jan 21, 2025
@srdas srdas added the bug Something isn't working label Jan 21, 2025
Copy link
Collaborator

@srdas srdas left a comment

Choose a reason for hiding this comment

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

The problem is that in Dark modes, the AI Settings pane does not render properly, as shown below:
image

Tested the fix to make sure it works for both dark themes:

image

and

image

Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@MUFFANUJ Thank you for contributing this, and welcome to the Jupyter AI project! 🤗 Reading from the CSS variables provided by JupyterLab to build a custom theme is exactly what we did in Jupyter AI v2. Your approach is correct, and I appreciate the time you took to write a description & provide a demo video as a new contributor. This is a great first contribution.

I did not mention that we still have the v2 implementation for setting the Material UI theme based on JupyterLab's theme. This JlThemeProvider component provides the styles through a React context, so all child elements are styled automatically without the need to add custom CSS to every element. The JlThemeProvider accidentally became unused during the initial v2 => v3 migration. Therefore, it would be better to re-introduce JlThemeProvider instead of adding custom CSS.

I've opened a new PR which performs these code changes: #1213. This provides a nicer UI with fewer lines of code by leveraging our existing work.

Since this is your first contribution: Feel free to copy my changes over into your PR, and I'll merge yours in favor of mine. It doesn't feel right to close the very first PR you opened for us, and I think you deserve credit for being the first to work on this issue.

@MUFFANUJ
Copy link
Contributor Author

MUFFANUJ commented Jan 22, 2025

Thank you so much for your kind words and suggestions @dlqqq @srdas ! 😊 I truly appreciate the time and effort you’ve taken to review my contribution, provide such detailed feedback, and even create an alternative PR.

I sincerely apologize for not getting complete clarity about the issue beforehand—it’s a lesson for me to always double-check and ensure alignment before starting. I’ve started understanding the codebase better little by little, and this experience has been incredibly helpful for my learning process.

I’ll go through the changes you’ve made in #1213 and study the approach with the JlThemeProvider to understand it more deeply. I’ll also keep your suggestion in mind for future contributions and will actively seek guidance from the community to make sure I’m aligned with the project goals.

Once again, thank you for your support and for making my first contribution such a positive experience.

@dlqqq dlqqq changed the title Fix: Jupyter AI's broken settings theme Fix MUI theme in Jupyter AI Settings Jan 22, 2025
Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

Awesome work, thank you! 🎉

No need to apologize about missing something; that's what code review is for. 🤗 Your PRs will get better if you read more code & understand what is happening.

@dlqqq dlqqq merged commit becc95c into jupyterlab:main Jan 22, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants