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

Adapt every UI component to auto scale in size #195

Closed
Reckless-Satoshi opened this issue Jul 30, 2022 · 10 comments
Closed

Adapt every UI component to auto scale in size #195

Reckless-Satoshi opened this issue Jul 30, 2022 · 10 comments
Assignees
Labels
⚡Eligible for Sats ⚡ This issue or pull request rewards bitcoin enhancement 🆙 New feature or request good first issue Good for newcomers

Comments

@Reckless-Satoshi
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
Most MUI components we are using can be scale in size up or down simply by changing the value of theme.typography.fontSize. However, the frontend currently has many hardcoded width and height props that we have implemented. If these were also relative to fontSize, it would trivial to adjust the size of the app, highly increasing the usability on phones with small screens. This enhancement will also help us reuse a few of the components in RoboSats PRO frontend (it will have a default smaller fontSize in order to pack more data in the screen).

Describe the solution you'd like
The default theme.typography.fontSize is 14, so for example if we change it to 12 the app will be zoomed out to ~85%.
https://github.com/Reckless-Satoshi/robosats/blob/aad87e7d98be30bbbb814b91534e0709724c2bc4/frontend/src/components/App.js#L26

lightTheme = createMuiTheme({
  typography: {
    fontSize: 12,
  },
});

However, when changing the size of elements this way, many of the hard-coded icons/images/boxes will remain the same size (... bad practices in our initial implementation). The result is a clunky looking frontend.

It would be great to make all of the hardcoded values automatically scale relative to props.theme.typography.fontSize. For that we will have to use withTheme in the class components and useTheme in the functional components. In addition, some of these values are directly hard-coded in static/index.css (for example the size of some Robot Avatars), ideally we should only rely on MUI theming.

This is a simple task, yet there might be dozens of hard-coded sizes all throughout the frontend. It will take reviewing and editing almost every file in /frontend/src/components then testing that the changes do actually lead to scalable components.

Additional context
This task is ⚡rewarded with 300K Sats⚡ . Drop a comment if you want to be assigned.

@Reckless-Satoshi Reckless-Satoshi added enhancement 🆙 New feature or request good first issue Good for newcomers ⚡Eligible for Sats ⚡ This issue or pull request rewards bitcoin labels Jul 30, 2022
@Reckless-Satoshi Reckless-Satoshi changed the title Adapt all UI component to auto scale in size Adapt every UI component to auto scale in size Jul 30, 2022
@fmitjans
Copy link
Contributor

Hi, I ran into a problem trying to use withTheme and useTheme. Importing any of these functions from the @mui/material/styles module will result in the following error:

MUI: withTheme is no longer exported from @mui/material/styles.
You have to import it from @mui/styles.
See https://mui.com/r/migration-v4/#mui-material-styles for more details.

However, @mui/styles is incompatible with React 18, as explained here: mui/material-ui#33066 (comment)

The suggested solution is to use @mui/system instead, but I'm not sure how to access the theme's fontsize with this module

@Reckless-Satoshi
Copy link
Collaborator Author

Reckless-Satoshi commented Aug 13, 2022

Hey @fmitjans ,

Thanks for implicitly taking over this task!

I will be giving it a try to get around this issue with the dependencies and get done one example in the following days. I'm sure there must be a way around this problem :) Hope it is not tricky to find!

@Reckless-Satoshi
Copy link
Collaborator Author

Reckless-Satoshi commented Aug 19, 2022

Hey @fmitjans ,

Indeed, there does not seem to be any clear solution. MaterialUI v5 seems to have dropped support for withTheme (needed for theming class components). I gave a try to package @mui/styles to no success (just as you documented).

The package @mui/system can be used for the functional components using the useTheme() hook. The use is detailed in the MUI docs

import { useTheme } from '@mui/material/styles';

function DeepChild() {
  const theme = useTheme();
  return <span>{`spacing ${theme.spacing}`}</span>;
}

However for the few class components left on the RoboSats app (the main big pages e.g. UserGenPage, OrderPage...), we need to find a workaround. I thought we could keep the theme dictionary as a state variable and, in addition of creating the theme (createTheme()), we can pass it as a prop to the children. Then it can be naturally accessed as this.props.theme.typography.fontSize.

I gave a try to this idea and created a new branch: main...demo-scale-ui-theme-size

On the first commit I created a simple "Zoom out button", when pressed it updates the state variable theme with a smaller fontSize. You can see the simple theme dictionary here: https://github.com/Reckless-Satoshi/robosats/blob/0e41f6c71d3525eb65286a2059a934114a6f40e4/frontend/src/components/App.js#L24
This button should suffice to live test most of the changes that need to be made to solve this issue.

As a demo of how we fix the hardcoded pixel sizes, I made the Robot avatar in the landing page scale in size with fontSize as well as the R logo. Accessing fontSize from theme App state variable:
https://github.com/Reckless-Satoshi/robosats/blob/0e41f6c71d3525eb65286a2059a934114a6f40e4/frontend/src/components/UserGenPage.js#L184
Dynamically resizing Robot avatar size using fontSize:
https://github.com/Reckless-Satoshi/robosats/blob/0e41f6c71d3525eb65286a2059a934114a6f40e4/frontend/src/components/UserGenPage.js#L204-L213
Dynamically resizing R svg logo using fontSize:
https://github.com/Reckless-Satoshi/robosats/blob/0e41f6c71d3525eb65286a2059a934114a6f40e4/frontend/src/components/UserGenPage.js#L322-L324

I hope this demo clears the way for the work needed to solve this issue.

I believe most functional components will be easy to adapt using the useTheme hook. And the class components can use the trick already implemented in the branch linked. I guess the hardest component to adapt will be the Order Book table (many hardcoded values, some interacting with each other).

Let me know if I can further help in some way.

@Reckless-Satoshi
Copy link
Collaborator Author

Hey @fmitjans , I wonder whether you are looking into this task. Should I assign you (no pressure, drop out if you want) so other devs can see someone is on top of this task already?

@fmitjans
Copy link
Contributor

fmitjans commented Aug 25, 2022

Sure, sign me up. Is it ok if I continue working on the demo branch?

@Reckless-Satoshi
Copy link
Collaborator Author

Sure, sign me up. Is it ok if I continue working on the demo branch?

Please continue working over it :)

@fmitjans
Copy link
Contributor

fmitjans commented Aug 27, 2022

I have a few question about the botom bar, which right now has fixed width and position:
https://github.com/Reckless-Satoshi/robosats/blob/0e41f6c71d3525eb65286a2059a934114a6f40e4/frontend/static/css/index.css#L61-L66

  1. How should the bar width behave when zoomed in? If the width is kept at 100% while the elements inside grow, they get cramped and overlap. Alternatively, the bar can be allowed to grow beyond the screen width (keeping the center inside the screen) in order to accomodate the elements inside, but some items would be left out of the screen (unless the bar is made to be scrollable).

  2. How should the bar vertical position change when zooming in? If the app is zoomed in too much, the bar starts blocking the view of the elements directly above (due to it growing in height). As long as the bar has a fixed positon at the bottom of the screen, the user can't scroll to get it out of view. I should note this is a problem only with a very large fontsize, so it depends on how much do we expect sizes to change.

Something similar should happen with other fixed components such as UnsafeAlert

@Reckless-Satoshi
Copy link
Collaborator Author

Reckless-Satoshi commented Aug 30, 2022

  • How should the bar width behave when zoomed in? If the width is kept at 100% while the elements inside grow, they get cramped and overlap. Alternatively, the bar can be allowed to grow beyond the screen width (keeping the center inside the screen) in order to accomodate the elements inside, but some items would be left out of the screen (unless the bar is made to be scrollable).

I guess at some point when increasing size, the Desktop BottomBar must be replaced by the Mobile one that is more sparse and has no text. Do you think this is possible?

  • How should the bar vertical position change when zooming in? If the app is zoomed in too much, the bar starts blocking the view of the elements directly above (due to it growing in height). As long as the bar has a fixed position at the bottom of the screen, the user can't scroll to get it out of view. I should note this is a problem only with a very large fontsize, so it depends on how much do we expect sizes to change.

I guess we will set a limit so the size cannot be increased much (Maybe 16?). Anyway, the scaling feature is for users to have some freedom to adapt to their needs, if they break the usability by increasing the size... maybe they went too far. Freedom brings responsibility :D

@fmitjans
Copy link
Contributor

Hello, I am thankful of having had the opportunity to contribute to this project, but unfortunately my circumstances have changed and I can't spare the time to complete this issue. I write this thinking it may be better to change the status of this issue in the rewards pannel, so as to avoid confusion.
I haven't really worked on any changes since my last PR, so not updates are necessary.

@Reckless-Satoshi
Copy link
Collaborator Author

Hello, I am thankful of having had the opportunity to contribute to this project, but unfortunately my circumstances have changed and I can't spare the time to complete this issue. I write this thinking it may be better to change the status of this issue in the rewards pannel, so as to avoid confusion. I haven't really worked on any changes since my last PR, so not updates are necessary.

Hey @fmitjans , thank you very much for letting us know and for having taken the time some time back to start this task. After the full redo of the frontend, this task is, in fact, already finished. Every UI component can be changed in size using the UI size slider on the Settings tab.

Once again thank you for your contribution! Closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚡Eligible for Sats ⚡ This issue or pull request rewards bitcoin enhancement 🆙 New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants