-
-
Notifications
You must be signed in to change notification settings - Fork 259
feat: Implement proportional symbol scaling for resizable output bar #2075
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
base: master
Are you sure you want to change the base?
feat: Implement proportional symbol scaling for resizable output bar #2075
Conversation
|
Hi!! its looks nice thanks for your contribution! |
|
Hi @tomivm, thanks for the review! I will fix the static code analysis (Codacy) errors and update the PR shortly. Regarding the settings config: I see you asked @martinbedouret for his input. I will wait for the final decision from you both on whether that is needed before making changes to the logic. |
|
Hey @tomivm @martinbedouret ! 👋 |
|
@martinbedouret @tomivm — I’ve updated the PR description to include a demo video and the final technical details regarding Redux persistence and the fixed unit tests. Everything is now passing and ready for your final review! |
|
Awesome work @techwithmuzzu |
src/components/Board/Board.css
Outdated
| min-height: 70px; | ||
| height: 155px; | ||
| max-height: 20%; | ||
| min-height: 90px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you needed to modify the original output sizes and padding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified these values to make sure the board layout stays consistent when the expansion is active. I'll clean up the code, remove the duplicate property, and try to keep the original padding values as much as possible.
src/components/Board/Board.css
Outdated
|
|
||
| .Board__output.Board__output--expanded { | ||
| height: 33vh; | ||
| max-height: 35vh; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally we don't trust in this units because dont work good in mobile browsers. Did you tested the implementation in a phone emulator or similar?
The ideal will be to use % instead of vh . If not i think vdh can work in mobiles browsers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s a very good point about the address bar issues on mobile. While testing in the emulator, I noticed that setting the height to 33vh (1/3 of the screen) actually makes the output bar feel too large on mobile devices. It leaves very little room for the input symbols, making them look disproportionately small.
I'll experiment with a smaller percentage (perhaps 20-25%) or a max-height constraint to ensure the output is readable without sacrificing the usability of the input board. I'll test these variations using dvh to see which provides the best balance on small screens.
src/components/Board/Board.css
Outdated
| } | ||
|
|
||
| .Board__output.Board__output--expanded .SymbolOutput__value { | ||
| min-width: 20vh; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ideal will be to use % instead of vh . If not i think vdh can work in mobiles browsers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I will consider that I will try to implement it with % units if not dvh.
| enableOutputExpansionDescription: { | ||
| id: 'display.enableOutputExpansionDescription', | ||
| defaultMessage: | ||
| 'Show a toggle button to expand the output bar for better visibility.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is not a clear instruction to define new messages in the project sorry for that.
We need need to add this messages to src/translations/src/cboard.json file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for letting me know! I wasn't aware of the central translation file. I’ll move the new message string to src/translations/src/cboard.json to keep it consistent with the project's structure.
src/components/Board/Board.css
Outdated
| position: absolute; | ||
| bottom: 0px; | ||
| left: 50%; | ||
| transform: translateX(-50%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is strict that you need to position this as absolute?.
Also maybe is simpler to have the output always big if user have the setting enabled. (Maybe this toogle is requiered in the issue i am not sure of this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a great insight! I think I was so excited to add features for my first contribution that I didn't stop to think about it from a pure UX perspective. You're absolutely right—if the user has already enabled the feature in the settings, a second toggle on the board is redundant and adds unnecessary clutter.
I'll simplify the logic to make the output expanded by default when the setting is on. I'll also refactor the positioning using Flexbox to get rid of the absolute positioning as we discussed. Thanks for the guidance, I'm learning a lot through this review!
- Refactor output bar to use dvh units for better mobile stability. - Replace absolute positioning with Flexbox for image centering. - Add responsive media queries for mobile and tablet devices. - Increase expanded height to 25dvh (approx 1/4th screen). - Clean up redundant styles and comments. - Add unit tests for output expansion logic and update snapshots.
160583a to
3d88255
Compare
|
Hi @tomivm @martinbedouret , Thanks again for the guidance! I have updated the PR to address all the review comments:
The build is green and ready for your review! 🚀 |
Description
This PR implements a toggleable Expanded Output Bar feature. It allows users to scale up symbols and text in the output area for better visibility and accessibility. This is especially helpful for users with visual impairments or when building long, complex phrases.
Visual Demo
🎬 Click here to watch the Demo Video
Key Changes
Feature Logic & UI
Board__output--expandedclass.vhunits for font-size and width in the expanded state..Symbolto ensure images and labels remain vertically stacked and centered.white-space: normalin expanded mode to allow long labels to wrap correctly.Settings & Persistence
SwitchinSettings > Displayto enable/disable the expansion handle.App.reducer.jsand ensured full state persistence via Redux-Persist.Display.messages.js.Unit Testing
Testing Performed
CC: @martinbedouret @tomivm