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

Dev gui - Darkmode using HSL Hue #630

Draft
wants to merge 45 commits into
base: develop
Choose a base branch
from

Conversation

gigamaster
Copy link

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

  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • ♻️ Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Description

Feature: Improve overall app design (look and feel) #539

  1. Images svg optimization.

  2. Darkmode using HSL Hue degree on the color wheel from 0 to 360 ie. 0 is red, 120 is green, and 214 is blue.
    Default css var --hue: 214;

Todo : add an input range under "dark theme" in main menu and store Hue value in localStorage.

Related Tickets & Documents

WIP #539

Mobile & Desktop Screenshots/Recordings

livecodes-ui-wip

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentations?

  • πŸ““ docs (./docs)
  • πŸ“• storybook (./storybook)
  • πŸ“œ README.md
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

No.

[optional] What gif best describes this PR or how it makes you feel?

Dark Mode minimalist theming. Todo : rename CSS vars and improve colour scheme with high visual contrast.

styles/app.scss - change theme color HSL Hue degree on the color wheel from 0 to 360 ie. 0 is red, 120 is green, and 214 is blue.
Copy link

netlify bot commented Aug 27, 2024

βœ… Deploy Preview for livecodes ready!

Name Link
πŸ”¨ Latest commit b9b0d83
πŸ” Latest deploy log https://app.netlify.com/sites/livecodes/deploys/66ecc6d54c4ab40008ce82cf
😎 Deploy Preview https://deploy-preview-630--livecodes.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hatemhosny
Copy link
Collaborator

Thank you @gigamaster
That's very nice πŸ‘Œ

I will have a good look and let you know.

Meanwhile, please run npm run fix to fix linting and formatting errors. Then we can look later into failing e2e tests.

Very grateful. Thanks.

@gigamaster
Copy link
Author

Thank you @gigamaster That's very nice πŸ‘Œ

I will have a good look and let you know.

Meanwhile, please run npm run fix to fix linting and formatting errors. Then we can look later into failing e2e tests.

Very grateful. Thanks.

Hi @hatemhosny

I was surprise that the Draft pull request to share the work-in-progress did trigger automated workflows 🀨
The automated development workflows are great and the "Deploy Preview" is really 😎
The purpose of this Draft PR was to seek feedback, discuss design decisions, iterate, and refine the changes before a pull request.
For example, moving UI icons to CSS using mask to make it easier to customize colors and sizes.
Use HSL Hue, an input range and save preferences to localStorage or make custom color palettes.
Then, delete code comments based on decisions made and run tests.
Have fun. Cheers ^_^/

@hatemhosny
Copy link
Collaborator

No worries, @gigamaster

I understand this is still WIP. I did trigger the automated checks just to get an idea.

I will go through the changes and design decisions and give feedback.

One note for now:
A better way than manually storing this setting to localStorage, is to add a config option (e.g. themeAccent or themeHue). This automatically saves it locally, and also allows other scenarios, like setting it in query params and setting/getting it in SDK even during runtime.
That's an easy change that I can guide implementing.

Overall, I really like what you are doing here. I'm a bit busy with some personal commitments for the next few days. I will have a good look when I have a chance and get back to discuss with you here.

Thank you.

@hatemhosny
Copy link
Collaborator

Hi @gigamaster

I had a good look around. I very much like your approach.
Using HSL hue and CSS icons are very neat.
I like the ability to change the colors by changing a single CSS variable.
Great work. Thanks a lot.

I do have some comments. A few are related to UI & colors (e.g. contrast of text/images with background, etc.). I think I would keep these till you finish because you are probably going to improve them anyways. You are much better than me in this area.

Other comments:

  • I planned doing this for a while, maybe it's time now. I want to change all clickable elements to <button> elements (to improve semantics, improve accessibility and keyboard navigation). We can still use CSS icons by either changing the spans to buttons or putting the spans inside the buttons (which is still valid HTML).
  • e2e tests failed mainly because clicking the run button using this selector ([alt="Run"]) no longer works. We need to change the selector to something that would match any of these 2 buttons: this and that. Maybe add a data attribute.
  • Would you elaborate why we need to add these gitattributes? What was the problem you faced? Would * text=auto be enough? Please note that I use Windows, but also want to allow contributors using other OSes to work with no problems.
  • One last stylistic comment (just a personal opinion): I preferred darker color for menus and models like you did here, while keeping the hue for accents like hover, active items, buttons, etc. What do you think?

Otherwise, please go on. I do like your approach.
Obviously, please feel free to ask if you have any inquiries.
Thank you.

@gigamaster
Copy link
Author

Hi @hatemhosny

I'm glad for the feedback.

I had a good look around. I very much like your approach. Using HSL hue and CSS icons are very neat. I like the ability to change the colors by changing a single CSS variable. Great work. Thanks a lot.

I do have some comments. A few are related to UI & colors (e.g. contrast of text/images with background, etc.). I think I would keep these till you finish because you are probably going to improve them anyways. You are much better than me in this area.

Ok then lets try to keep it simple...

Other comments:

  • I planned doing this for a while, maybe it's time now. I want to change all clickable elements to <button> elements (to improve semantics, improve accessibility and keyboard navigation). We can still use CSS icons by either changing the spans to buttons or putting the spans inside the buttons (which is still valid HTML).

Great, I'll dig into the code to see how changes can be made.

  • e2e tests failed mainly because clicking the run button using this selector ([alt="Run"]) no longer works. We need to change the selector to something that would match any of these 2 buttons: this and that. Maybe add a data attribute.

Noted.

  • Would you elaborate why we need to add these gitattributes? What was the problem you faced? Would * text=auto be enough? Please note that I use Windows, but also want to allow contributors using other OSes to work with no problems.

Oohh... gitattributes was merged from my workspace mockup/prototype/test : android -> old imac -> windows
In general, if it works on the old machine, it should work fine everywhere. Guess, it can be removed.
I did not find any particular issue so far, livecodes setup is highly efficient.

  • One last stylistic comment (just a personal opinion): I preferred darker color for menus and models like you did here, while keeping the hue for accents like hover, active items, buttons, etc. What do you think?

Otherwise, please go on. I do like your approach. Obviously, please feel free to ask if you have any inquiries. Thank you.

Dark/light mode implements a color-layer for the livecodes app and color-hue on elements only.
I tried CSS variables inheritance and fallback from editor, but it turned out to be confusing since users can choose a different editor... Also, the settings menu is split with a new settings-ui menu.
The menus could be over the top i.e : file, settings, utils, help, more on this later.
Right now, the menu setttings and settings-ui could be placed next to the logo
while keeping Run and Share on the left.

Livecodes UI Modal

livecodes-ui-modal

Livecodes UI Dialog

livecodes-ui-dialog

Livecodes UI Menu Settings

livecodes-ui-menu

@hatemhosny
Copy link
Collaborator

Oh wow!
That looks beautiful πŸ’―

I tried CSS variables inheritance and fallback from editor, but it turned out to be confusing since users can choose a different editor

Yes, currently users can choose a light editorTheme with a dark app theme. We might change this later, but this is the current behavior.

Also, the settings menu is split with a new settings-ui menu.

This is reasonable. Actually the left column of the "App menu" is related to the current project (except the login/logout link). While the right column is related to overall app settings. So it may be reasonable to split them to 2 menus: "Project" and "App Settings" (also with login link)

The menus could be over the top i.e : file, settings, utils, help, more on this later.
Right now, the menu setttings and settings-ui could be placed next to the logo
while keeping Run and Share on the left.

I will need to see a demo of this to make up my mind. Please note that LiveCodes does not provide multi-file support, so having a menu called "file" would probably not be suitable. "Project" would be a more suitable name.

Whenever possible, please push your changes, so that I can try it in the deploy preview. It does not need to pass the tests for now.

Thank you :)

@hatemhosny
Copy link
Collaborator

@gigamaster
One more thing that is not urgent at all, but I thought it might be useful to tell you in case you want to take it into consideration.
There is a quite significant work being done here to provide i18n support in LiveCodes so that the app would be multi-lingual. That feature is approaching to be ready.

A lot of LiveCodes users are Arabic speakers. So it would be great if we can also offer RTL layout.

This does not need to be in this PR. Just wanted to give you heads up.

Thank you :)

@gigamaster
Copy link
Author

gigamaster commented Sep 2, 2024

@gigamaster One more thing that is not urgent at all, but I thought it might be useful to tell you in case you want to take it into consideration. There is a quite significant work being done here to provide i18n support in LiveCodes so that the app would be multi-lingual. That feature is approaching to be ready.

A lot of LiveCodes users are Arabic speakers. So it would be great if we can also offer RTL layout.

This does not need to be in this PR. Just wanted to give you heads up.

Thank you :)

Hi @hatemhosny

So, to avoid any issues with editor theme selection, dark mode applies only to the application UI layer. By the way, I also noticed that Luna console and notifications are configured with light theme by default - maybe this can be changed to match the selected editor dark theme and, or app darkmode !?

After a quick check of i18n support branch, it doesn't seem like it's a problem to adapt and merge the UI. You can focus on i18n and later merge dark mode. Let me know when it's ready for translations.

About RTL layout - css dir pseudo-class might do the trick, maybe similar to html.dark, ie. html[dir="rtl"] to provide optional direction. I'll check browser support and test.
https://caniuse.com/css-dir-pseudo
https://caniuse.com/?search=direction

@hatemhosny
Copy link
Collaborator

hatemhosny commented Sep 2, 2024

So, to avoid any issues with editor theme selection, dark mode applies only to the application UI layer.

Yes, let's keep it like this for now.

By the way, I also noticed that Luna console and notifications are configured with light theme by default

You are correct. I have opened a PR in your repo to fix that. You may want to customize notification colors here. You can get the active theme using getConfig().theme.

After a quick check of i18n support branch, it doesn't seem like it's a problem to adapt and merge the UI. You can focus on i18n and later merge dark mode. Let me know when it's ready for translations.

That's great. Thank you.

About RTL layout - css dir pseudo-class might do the trick, maybe similar to html.dark, ie. html[dir="rtl"] to provide optional direction. I'll check browser support and test.
https://caniuse.com/css-dir-pseudo
https://caniuse.com/?search=direction

@shadeed has a great guide about RTL styling here: https://rtlstyling.com/posts/rtl-styling

Thank you.

@gigamaster
Copy link
Author

I'll merge the fix ToolsPane and update.
Thanks for the prompt reply and @shadeed guide, it's really helpful ^_^/

fix(ToolsPane): fix console themes
add menus project, settings, help
remove CodeRunButton
mobile orientation landscape
assets base64 scroll
@gigamaster
Copy link
Author

gigamaster commented Sep 6, 2024

I'll merge the fix ToolsPane and update. Thanks for the prompt reply and @shadeed guide, it's really helpful ^_^/

UI darkmode
add menus project, settings, help
remove CodeRunButton
mobile orientation landscape
assets base64 scroll

@hatemhosny
Copy link
Collaborator

hatemhosny commented Sep 7, 2024

Thank you @gigamaster
I had a look around

UI darkmode
add menus project, settings, help

Yes, I see now what you mean. I think this is a nice layout. That's more organized.
We can continue this pathway.

The hint tooltip covers the first menu item. It needs to be moved.
May be change class="menu hint--bottom" to class="menu hint--right"

remove CodeRunButton
mobile orientation landscape

This was needed for small screens because of lack of space in the top bar. I see you have not yet implemented the design for mobile portrait orientation. So, I'll just wait to see how this evolves. We also have now the login button (which is no longer in menu).

assets base64 scroll

I'm not sure this is useful. It might even be a bit confusing for users to scroll through the very long data urls.
I cannot find a delete button for assets. Unrecognized file icon (e.g. csv) does not have enough contrast to background.

The embedded playgrounds now break (demo).
In embedded playgrounds I move the logo to the right and use it to open the current project in the full standalone app.
The selector is no longer able to find the element to it crashes. You can find selectors here. You probably need to preserve these or replace them with valid selectors.

The dark loading screen should work only in dark mode. At that time the app data have not been loaded yet.
We faced this issue while implementing translation for the loading text. The code for i18n feature provides a solution for that which we can use for this once the other is merged.

Please go on. I like the direction you are going.
Thank you.

@gigamaster
Copy link
Author

Thank you @gigamaster I had a look around

UI darkmode
add menus project, settings, help

Yes, I see now what you mean. I think this is a nice layout. That's more organized. We can continue this pathway.

The hint tooltip covers the first menu item. It needs to be moved. May be change class="menu hint--bottom" to class="menu hint--right"

remove CodeRunButton
mobile orientation landscape

This was needed for small screens because of lack of space in the top bar. I see you have not yet implemented the design for mobile portrait orientation. So, I'll just wait to see how this evolves. We also have now the login button (which is no longer in menu).

assets base64 scroll

I'm not sure this is useful. It might even be a bit confusing for users to scroll through the very long data urls. I cannot find a delete button for assets. Unrecognized file icon (e.g. csv) does not have enough contrast to background.

The embedded playgrounds now break (demo). In embedded playgrounds I move the logo to the right and use it to open the current project in the full standalone app. The selector is no longer able to find the element to it crashes. You can find selectors here. You probably need to preserve these or replace them with valid selectors.

The dark loading screen should work only in dark mode. At that time the app data have not been loaded yet. We faced this issue while implementing translation for the loading text. The code for i18n feature provides a solution for that which we can use for this once the other is merged.

Please go on. I like the direction you are going. Thank you.

Hi @hatemhosny

The main idea is to make the user interface layout match the modern web apps design that Internet users are familiar with.
If visuals are important, we remember more about how a web app helped to achieve our goals, and how easy it was to do it. Let's say you are on the go, on a train or plane, and want to elaborate an idea, the choice would be pencil and paper or "livecodes". The goal is then to maximize the area of code and minimize the menus to provide quick access to features (spacing items to later customize with icons and keyboard shortcuts). I thought a tools menu could be useful to move the asset manager, code snippets and allow other add-ons, e.g. CSS color and unit conversion, cloud storage (link, modal or popup), more on this later.

The base64 asset scrolling was an attempt to fix the very long string after adding an image. While I try to avoid touching the core files as much as possible, I noticed that modals have a different HTML element structure that can't be solved by CSS alone, same for tabindex and flex ordering to facilitate LTR and RTL.

Some buttons don't show up, but I find them useful, for example, split and full screen.
Are they supposed to be used only on the embedded playground?

Thanks for reporting the issues - I'll check that out further.
Have fun ^_^/

@gigamaster
Copy link
Author

gigamaster commented Sep 7, 2024

  • Todo nav - menu

ConsistencyNot conventional
Use hr instead of the separator role to ensure accessibility across all devices.

Validation Error: Element hr not allowed as child of element ul
HR shouldn't be recommended since it's invalid.

However, the li[role="separator"] is allowed in HTML 5.2
Ref.:
https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-editor/
https://www.w3.org/TR/html52/grouping-content.html#the-li-element

Issue:
whatwg/html#9247

listitem role (default - do not set), option, presentation, radio, separator, tab or treeitem.

@hatemhosny
Copy link
Collaborator

hatemhosny commented Sep 7, 2024

The main idea is to make the user interface layout match the modern web apps design that Internet users are familiar with.
If visuals are important, we remember more about how a web app helped to achieve our goals, and how easy it was to do it. Let's say you are on the go, on a train or plane, and want to elaborate an idea, the choice would be pencil and paper or "livecodes". The goal is then to maximize the area of code and minimize the menus to provide quick access to features (spacing items to later customize with icons and keyboard shortcuts). I thought a tools menu could be useful to move the asset manager, code snippets and allow other add-ons, e.g. CSS color and unit conversion, cloud storage (link, modal or popup), more on this later.

I do agree on the concept and approach. I think this is indeed more organized.

The base64 asset scrolling was an attempt to fix the very long string after adding an image. While I try to avoid touching the core files as much as possible, I noticed that modals have a different HTML element structure that can't be solved by CSS alone, same for tabindex and flex ordering to facilitate LTR and RTL.

I do not mind introducing some changes in HTML, so long that they do not break the app.
I just mean that showing the full data url is not useful to the user and may be confusing. No body would actually need to read the full data URL in contrast to other regular URLs which might be useful (e.g. when uploaded to github pages)
Another way of doing this would be to trim the URL so that it spans only 1 line, like the current implementation.
I also tried to make it look similar to the project list screen (Open ...), for familiarity and consistency.

image

The embedded playgrounds now break (demo).

It now works. Thank you.

Some buttons don't show up, but I find them useful, for example, split and full screen.
Are they supposed to be used only on the embedded playground?

Yes. Embedded playgrounds have limited space (inside the embedding page). So it would be useful to see the playground in full screen to have more room without taking up more space from the embedding page. In contrast, the full app already takes the full view port and can be easily viewed in full screen by pressing F11 (which is not available for embeds).
Same goes for the split toggle button. It is easier to manually resize in the full app.

On the other hand, all menus and login button should not be shown in embeds. I do not even attach event handlers to them.
It should be focused on just showing the UI necessary to edit some code and see the result. In case the user wants to do more, he can click on the logo (with the hint: edit in LiveCodes), which would take him to the full app with his project loaded.

This is the current view for embeds. We should hide all menus and the login button:

image

I noticed that changing the editor causes some flacky movement in the editor toolbar below:

Screenity.video.-.Sep.7.2024.mp4

However, the li[role="separator"] is allowed in HTML 5.2

I do like the menu separators. They make things a lot more organized and also look great.
Thank you.

@hatemhosny
Copy link
Collaborator

One more thing. The tooltip in the lowest menu item tries to appear below but overflows to the next column.
Maybe we should change all tooltips to appear on the right class="hint--right", like you did with the menu labels.
Also for changing language.

image

@gigamaster
Copy link
Author

gigamaster commented Sep 8, 2024

  • App Menu ( spacing + flex)
  • App Menu tooltip (language-item hint left position)
  • Embed playground (remove menus + login)
  • Console tests (result, fail css var)
  • Console luna count (text color)
  • Fixed editor toolbar (position absolute)
  • Reverse asset manager + add action delete
  • Removed .gitattributes (CRLF/LF)

WIP

  • Dark & Light mode color scheme
  • CSS vars
  • LTR - RTL
    • Extra margin in flexbox - Browser bug ??
    • Menu select-editor
    • Position absolute (left-right)
  • i18n
  • Tests

@hatemhosny
Copy link
Collaborator

@shadeed has a great guide about RTL styling here: https://rtlstyling.com/posts/rtl-styling

@shadeed will be giving a talk about RTL styling in CSS
https://x.com/shadeed9/status/1836350507264221311

@gigamaster
Copy link
Author

@shadeed has a great guide about RTL styling here: https://rtlstyling.com/posts/rtl-styling

@shadeed will be giving a talk about RTL styling in CSS https://x.com/shadeed9/status/1836350507264221311

Livecodes UI vs Editors LTR - RTL

Issue
if UI direction is RTL the monaco editor must be forced to LTR

Support for RTL editing #2371
https://microsoft.github.io/monaco-editor/typedoc/enums/SelectionDirection.html#RTL
"We closed this issue because we don't plan to address it in the foreseeable future."

Support for RTL languages (such as Arabic / Hebrew / Persian etc.)
microsoft/vscode#11770
an extention to make the code editor go from ltr to rtl with one line or all lines
https://github.com/SarbandAkray/RTL

@hatemhosny
Copy link
Collaborator

The RTL UI is nice πŸ‘

Now, the dir attribute is added to the <html> element when setting the i18n language:
https://github.com/zyf722/livecodes/blob/bbd6496743b3b061f4daafadd465577e9dcce4f9/src/livecodes/core.ts#L4165

Issue if UI direction is RTL the monaco editor must be forced to LTR

Support for RTL editing #2371 https://microsoft.github.io/monaco-editor/typedoc/enums/SelectionDirection.html#RTL "We closed this issue because we don't plan to address it in the foreseeable future."

Support for RTL languages (such as Arabic / Hebrew / Persian etc.) microsoft/vscode#11770 an extention to make the code editor go from ltr to rtl with one line or all lines https://github.com/SarbandAkray/RTL

I do not think we should try to turn the editor direction to RTL.
Whatever the UI language is, the code is still LTR.
I think we should just let this be as it is.

Copy link

sonarcloud bot commented Sep 20, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
6 Security Hotspots
26.7% Duplication on New Code (required ≀ 3%)
C Reliability Rating on New Code (required β‰₯ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

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