-
-
Notifications
You must be signed in to change notification settings - Fork 830
Conversation
this.setState({ | ||
interfaceScale: newInterfaceScale, | ||
}); | ||
PlatformPeg.get().reload(); |
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.
Changing scale yielding a full reload is really bad UX :(
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.
Yes. Just a copy from language setting to start with. What would you prefer happen? Options:
- Note that zoom will apply after a restart of the app
- Zoom as the setting is changed - might be buggy because of scaling happening while the user is touching controls
- Zoom with a delay?
If 2 is preferable, is there a way in this sdk to watch a state change (scale changing) on the root element? Maybe SettingsManager
has a watch function? Or is the only way to bubble up the change through the component tree?
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.
you could use the dispatcher to send a zoom_change
event that LoggedInView/MatrixChat listens on and acts on when you save the setting
@t3chguy let me know what you think. Not quite sure what to do about translations. |
@t3chguy Any chance you could have another peek at this one? Let me know if it's not required anymore or you'd like to see something different. Thanks 🙂 |
I'll take a look at it tomorrow |
} | ||
} | ||
|
||
InterfaceScaleSlider.propTypes = { |
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.
this can be moved into the body of the class as
static propTypes = {
...
};
this._onValueChange = this._onValueChange.bind(this); | ||
} | ||
|
||
componentWillMount() { |
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.
no point having an empty willMount
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.
Sorry about the goofs 🙂 updated those now
} | ||
|
||
render() { | ||
const interfaceScale = SettingsStore.getValue("interfaceScale", null, /*excludeDefault:*/true); |
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.
comment?
@@ -0,0 +1,50 @@ | |||
/* | |||
Copyright 2017 Marcel Radzio (MTRNord) | |||
Copyright 2017 Vector Creations Ltd. |
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.
^^ this needs fixing
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.
What should this be changed to? This was a copy from another component. Should I be putting it under my own name?
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.
Yup, unless someone else wrote it :P
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.
ok 😄 and also remove Copyright 2017 Vector Creations
?
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.
yup :)
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.
done 😃 hope my name is not taking too much space in the repo
|
||
export default class InterfaceScaleSlider extends React.Component { | ||
propTypes = { | ||
className: React.PropTypes.string, |
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.
plz use import PropTypes from 'prop-types'
and then replace React.PropTypes
with PropTypes
Then you also need to sign-off, described here: https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.rst |
will test and throw at toml to play with tomorrow :) |
@t3chguy great 😃 . There are various methods for the UI as seen in the screenshots in element-hq/element-web#3160. The slider seemed more flexible but other ones may work as well. Feel free to let me know if you think another method is more appropriate. |
@t3chguy cool. Well it is a slider but it isn't that fancy 😄 I can see tomorrow what could be added to make it a bit more appealing. |
|
||
|
||
export default class InterfaceScaleSlider extends React.Component { | ||
propTypes = { |
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.
missing word static
should be static propTypes
I showed it to @lampholder - I think he had a few comments about it |
@t3chguy Happy to hear 🙂 |
On possibly using a library for the slider itself, react-rangeslider and rc-slider seem pretty decent. Styles customisable + labels like discord has. |
My thoughts so far:
Also, it doesn't seem like the setting is being persisted beyond the user logging out/in. The challenge is that the optimal UI-scalaing feature needs to meet two use cases, with different objectives.
So I can see why Discord adopted the independent chat text scaling slider. More thoughts to come (but wanted to show that I am actively considering this issue right now 😄 ) |
Okay, in terms of what needs to be done to deliver the optimal UX/text scaling solution (which is separate to the question of what needs to be done in order to claim the bounty, which I'll consider afterwards), I think we need:
|
Thanks for the input @lampholder I'm with you on the need for a separate font size scaling slider.
👍 agreed, jumped out at me too.
This should be working but might be persisted at the wrong level - I will double check this.
This is to prevent the zoom control falling out of the users view while they are scrolling it but it can also be solved be applying the zoom after the user has confirmed a selection so will change it to this.
Styled sliders are a tricky thing cross browser. This would come with the consequence of a new react component dependency and/or substantial new code. Are you happy with this? Any preferences? I'm happy to continue to contribute to this regardless of what would be in scope for the bounty |
Hi @monokh - hope you had a good break/new year :) Thanks for you ongoing patience with this! Responding to your points:
Excellent :)
I think we should solve this as a totally separate issue (you're more than welcome to resolve it, but I think it's beyond the scope of this original bounty). Actually, it is a totally separate issue; here: element-hq/element-web#4805. There's already some CSS kicking around for this (wrapped up even in a neat userscript/userstyle in the discussion on element-hq/element-web#1371).
This might be me being a crank - it looks like the changes are persisted across different sessions on the same browser, which I think is probably the desired behaviour here (i.e. change made in Chrome on your laptop will be there the next time you access Riot from that browser, but not from Chrome on your desktop/etc.)
Yeah, I think that'll work - playing around with it now I think it would be fine if it just applied the changes as soon as the user 'drops' the slider.
Good point - is there simple styling that can be applied cross browser trivially? I'd rather we not drag in significant dependencies/chunks of code if we can avoid it. |
@lampholder Happy new year 😃
cool 🙂 well I can attend to it through there then.
Will see about doing that!
I can't really think of a super simple middle ground besides positioning and extending it to full width. Perhaps just using a sass module (or even importing the required rules into the project) is the leanest approach? This sass module is pretty commonly used. While it doesn't have pretty labels, the slider itself can be styled to fit the riot brand pretty easily and it's not a substantial amount of code or a heavy dependency. |
@lampholder Sorry about this going slowly, just been chipping away at this when i get some free time. There may be some follow up style changes to make sure everything scales well at the different breakpoints but i fixed anything that stood out anyway. Let me know what you think :) |
@lampholder @t3chguy Any chance you could have another look at this one? Keen to get this finished eventually. |
I'll pester him about it 2PM Thursday |
@t3chguy Yep. As i mentioned before, there may be some elements that don't scale correctly but they are easily fixable. When you are happy with the general state of the feature, I can do a more thorough correction on the styles in element-hq/element-web#5931 to make sure everything scales correctly. |
I built it for toml to have a play with: https://riot.ovh/builds/tomltomltoml/ |
Any chance for another look at this? Seems we are pretty close 🙂 |
Checking back on this one. Are you happy with the general implementation as demonstrated on the demo link? I'm looking forward to pushing this over the line. |
@t3chguy can you look at this again now that you are back? |
I tried https://riot.ovh/builds/tomltomltoml/ there are two zoom settings, but only the one for font-size seems to wirk, the other has no effect (tested on firefox 63.0 (64-Bit), Ubuntu 18.04) |
Thanks for making this contribution a while back. Since the code base has changed since this was opened, it no longer applies cleanly, and I don't think there's a need to keep it open in this state, as we can always find the code here again if needed. If you are still interested in pursuing this feature, please discuss with us in #riot-dev:matrix.org to find a good approach forward. |
Work towards an interface scale setting.
Fixes element-hq/element-web#3160
Signed-off-by: Mohammad Nokhbatolfoghahai [email protected]
TODO:
onMouseUp
)