Font Setting - Added draggable slider #10126
Conversation
Signed-off-by: Shubham-Rasal <bluequbits@gmail.com>
github-actions
bot
added
the
Z-Community-PR
Shubham-Rasal
changed the title
github-actions
bot
added
the
T-Enhancement
Shubham-Rasal
changed the title
| if (!innerDot) return null; | ||
| // innerDot.style.left = `${this.state.percent}%`; | ||
| const offset = this.offset(this.props.values, this.props.values[value]); | ||
| innerDot.style.left = `calc(-1.195em + " + ${offset} + "%)`; |
There was a problem hiding this comment.
What does -1.195em relate to? How could we avoid that magic number?
If and we can't avoid it, we should add a thorough comment explaining how we got there
| if (!target.parentNode) return null; | ||
| const parent = target.parentNode as HTMLElement; | ||
| if (!parent.parentNode) return null; | ||
| const slider = parent.parentNode as HTMLElement; |
There was a problem hiding this comment.
We should use the non-null operand here
| const slider = parent.parentNode as HTMLElement; | |
| const slider = parent.parentNode!; |
| const target = e.target as HTMLElement; | ||
| if (!target.parentNode) return null; | ||
| const parent = target.parentNode as HTMLElement; | ||
| if (!parent.parentNode) return null; | ||
| const slider = parent.parentNode as HTMLElement; |
There was a problem hiding this comment.
I believe we should rely on react refs instead of traversing the DOM.
The advantage I see is that we explicitely declare refs on the DOM element we wish to track.
With your approach, we rely on the fact that the DOM structure, and that is subject to change in the future
| @@ -83,18 +89,73 @@ export default class Slider extends React.Component<IProps> { | |||
|
|
|||
| let selection = null; | |||
|
|
|||
| const onDrag = (e: React.DragEvent) => { | |||
There was a problem hiding this comment.
There's a lot of logic overlap on onDrag and onDragEnd could we consolidate those ?
There was a problem hiding this comment.
Even I had that in my mind. Will make a separate function for common operations.
removed comments Co-authored-by: Germain <germain@souquet.com>
removed console.log comment Co-authored-by: Germain <germain@souquet.com>
Co-authored-by: Germain <germain@souquet.com>
|
@Shubham-Rasal if this is ready for review again, please click the little ♻️ button next to @gsouquet 's name |
Sorry for the delay but I m not able figure out how I could refactor this to give the same output. Also a took a break for a while, sorry for that. In the mean time if anyone wants to claim this issue, they are free to do so. |
|
No problem at all @Shubham-Rasal - thank you for your work on this. I'm going to mark it as draft for now, since it's not ready at the moment. |
Signed-off-by: Shubham-Rasal bluequbits@gmail.com
Checklist
element-web notes: none
Type: enhancement
Fixes vector-im/element-web/issues/20168
In element/web the font slider in the user settings is not draggable. This is a natural user response to drag a slider , but here it just selects the text.
This is the before image as mentioned in the issue:
This is the result after the changes :
Element._.testing.room.-.Brave.2023-02-09.22-36-21.mp4
Here's what your changelog entry will look like:
✨ Features