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

Current progress on issue #195 (auto-scale to fontsize) #233

Merged
merged 6 commits into from
Sep 9, 2022

Conversation

fmitjans
Copy link
Contributor

@fmitjans fmitjans commented Sep 9, 2022

As discussed on the telegram group, this is my partial progress for issue #195.
I managed to merge and rebase my changes over the latest main commit, and everything seems to work fine when running the frontend.

One small issue: I noticed the Expiry Timers section on page /make > customize doesn't show the option to pick times*.
The latest main commit also had this problem, and my branch showed it just fine (branching from commit 7fb2767), so this is probably caused by one of the latest commits. I thought it would still be better to rebase my changes to the latest commit instead of an old one.

*Specifically: this section is not showing:

https://github.com/Reckless-Satoshi/robosats/blob/645671265ab19d3fd127fa08f21dd76e9ebfa423/frontend/src/components/MakerPage.js#L601-L665

return (
<Router >
<div className='appCenter'>
<div className='appCenter' style={{height: window.innerHeight - 40*fontSizeFactor - 20, overflow:'auto'}}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaks the UI as the app is not vertically centered and the height is too tight (shows annoying scroll bar on the middle right side)

@Reckless-Satoshi
Copy link
Collaborator

Amazing! Thanks for this. I am temporarily hiding again the zoom-in and zoom-out buttons on the main branch. I also am restoring the original AutocompletePayments.js, as I do not understand well why it looks different.

Good catch for the Expiry Timers. I guess some dependency got lost (yet it does not spell any error... weird!). Fixed it before merge.

I noticed the maker page does not really scale in size. Do you know why? Also noticed many sizes in "em" units instead of "px" does using "em" solves the austcaling issue without need to use fontSize theme variable?

This work counts for ~half the reward on the Dev Rewards panel. Issue the invoice with the final PR :D

@Reckless-Satoshi Reckless-Satoshi merged commit af062a4 into RoboSats:main Sep 9, 2022
@fmitjans
Copy link
Contributor Author

fmitjans commented Sep 10, 2022

does using "em" solves the autoscaling issue without need to use fontSize theme variable?

Yes, I realized that the 'em' unit changes with the page font size (unlike the fixed 'px' unit), so I figured it would be a better solution than passing fontSize as a prop, which increases the code's complexity, specially with nested components. I checked with the element inspector and it has the same effect as the original solution

I noticed the maker page does not really scale in size. Do you know why?

I'm not sure what you mean. It does scale in a weird way, because many elements in the maker page had fixed width and unspecified height, so I only changed their width to a relative one (using 'em'), as a result the page scales more to the sides than in height. If this is what you mean, I can specify every size so that the page scales as if zoomed, it shouldn't be hard.
The page should definitely scale somewhat, if it doesn't change at least a little there may be an error.

I also am restoring the original AutocompletePayments.js, as I do not understand well why it looks different.

The AutoCompletePayments component is part of the maker page. Before the changes it didn't change in sizes along with the rest of the page, so for example, if you zoomed in, you can see the list moving gradually to the left. In any case, this effect is small and it might be difficult to notice without the page inspector (if you inspect the div or the svgs in the payments autocomplete section, you can see the size stays fixed while the rest of the page changes, this doesn't happen when the commit is applied). I felt like it would be better to change everything to relative sizes now rather than leave it hardcoded
The page still works fine without these changes though, I'll leave the decision to you to keep them or not

@Reckless-Satoshi
Copy link
Collaborator

Reckless-Satoshi commented Sep 12, 2022

I checked with the element inspector and it has the same effect as the original solution

Awesome!

I'm not sure what you mean. It does scale in a weird way, because many elements in the maker page had fixed width and unspecified height, so I only changed their width to a relative one (using 'em'), as a result the page scales more to the sides than in height. If this is what you mean, I can specify every size so that the page scales as if zoomed, it shouldn't be hard.

When I tested it, the makerpage was not scaling at all. But I discovered that it had to do with using the Dark theme. I fixed the conflict between light/dark theme and the fontSize in App.js in one of the latest commit.

I felt like it would be better to change everything to relative sizes now rather than leave it hardcoded
The page still works fine without these changes though, I'll leave the decision to you to keep them or not

If using em instead of px is a general solution, let's use em everywhere as the unit and write down all sizes in "em" (without the fractions).

Since the makerpage did not zoom in or out for me, the only thing weird I noticed is the AutocompletePayments was less wide than usual.

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.

None yet

2 participants