-
-
Notifications
You must be signed in to change notification settings - Fork 186
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
feat(website): add stretchable layout for playground #2067
feat(website): add stretchable layout for playground #2067
Conversation
✅ Deploy Preview for modelina ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Pull Request Test Coverage Report for Build 10310584171Details
💛 - Coveralls |
@devilkiller-ag To accommodate max/min width I need to adjust the position of the layout and put output options on the left side of the panel, also I am not sure about the number for now I put 20% minimum appearance for either input/output editor, did you have an opinion about this one? |
…able and store them
…n the global state
@devilkiller-ag can I get a review or feedback at least? it's been a while |
Hi @jerensl Apologies for the late review. I got stuck with some work. I will do the review by tomorrow. |
Take your time, I did it just for visibility because sometimes also notifications will not appear on me, and lose track of some PR. No need to hurry :) |
We can put a 30% minimum appearance because the input editor generally will have a deeply nested code, and its formatting keeps most code lines after 3-4 tab spaces. Can you try it if it looks good or not? |
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 code changes look good to me. Can you change the minimum appearance for the input/output editor to 30% and check? Also, why do we need to use immer
?
@@ -0,0 +1,153 @@ | |||
import type { Draft, Immutable } from 'immer'; |
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 do we need 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.
It's one of react design's core principles which is immutability in state, sometimes when the state is not immutable it also doesn't trigger the re-rendering process, the state is updated but not changed on the UI
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.
Also, object reference uses heap allocation when the object property value changes, it's still using the same memory address
Is it feasible to add this accessibility feature? Again sorry for the late review, I was stuck in some work the past two weeks. |
… into feat/stretchable-layout merge from master
I'm not sure either, but I'm already implementing some semantic and basic aria roles as well. Just not be sure how much it helps and how to measure it precisely
No worries |
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.
In the mobile view, When accessing the output navigation
(last icon) from editor options
, it is always hidden by the opened-up editor options. So, for mobile view, I suggest either hiding its icon when the editor option is open or switching it to the output screen with the output option
opened up on clicking its icon. Rest everything seems perfect to me.
@jonaslagoni what's your view on 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.
In the mobile view, When accessing the output navigation (last icon) from editor options, it is always hidden by the opened-up editor options. So, for mobile view, I suggest either hiding its icon when the editor option is open or switching it to the output screen with the output option opened up on clicking its icon. Rest everything seems perfect to me.
@jonaslagoni what's your view on this?
I agree, never have a button unclickable ✌️
Other than that this looks good ✌️
@jerensl seem like all this needs is removing the unclickable buttons 🤔 |
Signed-off-by: jerensl <54782057+jerensl@users.noreply.github.com>
… into feat/stretchable-layout
@devilkiller-ag @devilkiller-ag, I apologize for the late response, I already made changes for mobile and hid the output options when the user clicked on the general options, what do you think? |
Quality Gate passedIssues Measures |
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.
Looks good to me :)
@devilkiller-ag anything from you? |
@jonaslagoni All good from my side too!! I was just waiting for the deployment preview to work. Thanks, @jerensl for working on this issue ✨ |
/rtm |
🎉 This PR is included in version 3.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Overview
In the context of stretchable editor space playground, I decided to use framer-motion because they really care about accesibility and performance, why I really like working this library is how they're just giving us some utility function which can be expanded for broader use cases, I love that because it's enabled us as a developer to be more creative when dealing with animation and interaction.
For the stretchable alone I'm using useTransform, it's seems counterintuitive in the first place but after you understand it's just a pub sub-model, it's a bit clever how they were implementing it that way.
In the context of state management, I am using useReducer purely because of familiarity with the built-in state management by React, I am also using Immer for maintaining an immutability state which is an good pattern in react when the state must be immutable and not directly changed, it also enables us to work in a complex nested state, I'm also using typescript to prevent any mutation outside of useReducer
Functional Requirements
Non-Functional Requirements
Related Issue
Resolved #1846
Additional Notes