Closed rajkumardongre closed 1 year ago
Thanks @rajkumardongre, I'll review it.
Can you please review and remove the files that are committed and are unrelated to this feature?
At least the ones in backend/ (they shouldn't be committed), but also package-lock.json (let's commit it in a separate PR).
Do you think it would make sense to also change the theme (dark/light) of the rest of the interface, or editor only as it is now?
Great PR @rajkumardongre. Thanks for this.
I have following doubts here:
1200 x 1200
(and are of size ~18Kb
) where as the existing icon images are of the resolution 24 x 24
(and are of size ~200-500
bytes). Does using higher resolution images (and thus larger size) affect the site performance? (for example: delayed loading as more data needs to be fetched)Thanks @rajkumardongre, I'll review it.
Can you please review and remove the files that are committed and are unrelated to this feature?
At least the ones in backend/ (they shouldn't be committed), but also package-lock.json (let's commit it in a separate PR).
Do you think it would make sense to also change the theme (dark/light) of the rest of the interface, or editor only as it is now?
Thank you @milancurcic for your suggestion.
ok, i will remove the unrelated files from my commit
Yes, I think it will be great if we can change the theme of the whole UI (either dark or light).
Theme Toggle Demo
Light mode
Dark mode
Great PR @rajkumardongre. Thanks for this.
I have following doubts here:
- Do we need to be concerned about the sources of the images for the dark mode and light mode icons? For example, are there any copyrights for the icons used or is there any attribution required to use these icons?
- The resolution of the images used is around
1200 x 1200
(and are of size~18Kb
) where as the existing icon images are of the resolution24 x 24
(and are of size~200-500
bytes). Does using higher resolution images (and thus larger size) affect the site performance? (for example delayed loading as more data needs to be fetched)
Thank you @Shaikh-Ubaid for your suggestions.
I go through the images' websites and found that the image will require attributions. should i change the images?
Yes, large image size can affect the site's performance, I will try to compress it.
Once again Thankyou @milancurcic and @Shaikh-Ubaid for your valuable reviews. I go through all your suggestions and accordingly have made changes to them, and made a PR. Changes :
Kindly can you Review the Code.
Thanks for the updates.
A tangential comment about icons: I noticed that whenever we add new icons we introduce new image files for the icons. Would it not be a better solution to rely on an icons library? Since we're using Bootstrap for UI elements, we could use Bootstrap Icons library for button icons. Nothing to do about this in this PR, just wanted to bring it up.
I noticed that whenever we add new icons we introduce new image files for the icons. Would it not be a better solution to rely on an icons library? Since we're using Bootstrap for UI elements, we could use Bootstrap Icons library for button icons.
That's a great idea @milancurcic! Thanks for sharing! I created an issue for it at https://github.com/fortran-lang/playground/issues/64
Yes, it will be great to add Icons, instead of images because as the application will grow we will require more icons, therefore shifting to Bootstrap Icon will be a great move in perspective of expanding our application.
Amazing work @rajkumardongre. I really appreciate it. Thank you so much for it!
A last doubt from my side: Do you think it would be possible to commit the changes which support the theme toggling feature and formatting (indentation/adding-semicolons) changes in different commits (or ideally separate PRs)? Currently, since both formatting changes and theme toggling changes are together, it seems tricky to see exactly which changes were made.
Thanks @Shaikh-Ubaid for your appreciation, actually formatting changes(like adding indentation/semicolons/double quotes) was not intentional, I think while saving the code, my code Editor(VS Code) just-auto formatted it, by itself.
Hello, is there any changes that i should make in my pull request, or add any new feature to it?
Thanks, @rajkumardongre for this update. It looks great.
Unfortunately, the "Layout" button icon looks a lot like "Run". Rather than searching for alternative icons (I did a brief search in the Bootstrap-icons gallery and can't find anything better), for now can you just re-arange the buttons in this order:
Run -> Reset -> Libraries -> Layout -> Light/Dark
After that change, this will be good to merge.
Hi @milancurcic, thanks for your review I have rearranged the button order and I also go through many online code Editor and found that most of the online Editor have a green color button for running the code, so inspired by this I also change the background color of the run button to green. I think this will make our user experience more smooth
Added a Toggle button on UI which Enable user to go from dark mode to Light mode and vice versa
For Dark Mode
monokai
theme is used For Light Modesolarized_light
theme is used as mention in the issue.Dark Mode :
Light Mode :