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

feat: improve UI responsiveness #95

Closed
wants to merge 12 commits into from
Closed

Conversation

CarmitKl
Copy link

@CarmitKl CarmitKl commented Mar 23, 2022

Implement UI support for small screens, like laptops and tablets. Smallest value: 600px. On this PR the toasts are overlapping the menu for vertical tablets. Horizontal tablets are without that overlap.

This change is related to this issue:
#26


This change is Reviewable

@CarmitKl CarmitKl requested a review from dan-ziv March 23, 2022 10:51
@CarmitKl CarmitKl added the ready for review Pull request is ready to be reviewed label Mar 23, 2022
src/enums/Breakpoints.js Outdated Show resolved Hide resolved
src/components/Containers/Header/Header.module.scss Outdated Show resolved Hide resolved
src/components/Features/ToastProvider/ToastProvider.js Outdated Show resolved Hide resolved
src/components/Features/ToastProvider/ToastProvider.js Outdated Show resolved Hide resolved
src/components/Features/ToastProvider/ToastProvider.js Outdated Show resolved Hide resolved
@@ -12,6 +12,7 @@ export const Button = ({
height,
icon,
iconAlign = 'left',
buttonClass,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you added an option to send class prop, why buttonClass as single and not just classes as array?

Copy link
Author

Choose a reason for hiding this comment

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

currently, that's the only class a button gets in the whole project.
also, there's an extended usage of a util function that gets an array of values and returns a string.

@@ -28,4 +30,12 @@
svg {
margin-right: 10px;
}

&[class~='wallet-btn'] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change to scss syntax

Copy link
Author

Choose a reason for hiding this comment

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

writing: &.wallet-btn doesn't seem to work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

1.It won't work - CSS modules add id to each class at runtime. You should use the styles object of the CSS file imported to the component. i.e., styles.walletBtn.
2. Why are you adding app-specific class component styles (wallet-btn) to a generic component styles?

src/components/UI/WalletButton/WalletButton.js Outdated Show resolved Hide resolved
src/styles/breakpoints.module.scss Outdated Show resolved Hide resolved
Comment on lines 7 to +10

:export {
mainOffset: $--main-offset;
mainOffsetSmall: $--main-offset-small;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is small? tablet? mobile? please write the appropriate breakpoint.

Copy link
Author

Choose a reason for hiding this comment

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

the '$--header-height' is for width 1280 and above. breakpoint DESKTOP.
and the '$--header-height-small' is for all the rest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So why not to change $--header-height to $--header-height-desktop and $--header-height-small to $--header-height???

Copy link
Collaborator

@dan-ziv dan-ziv left a comment

Choose a reason for hiding this comment

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

Revert the disclaimer position to left

@CarmitKl CarmitKl added IN PROGRESS and removed ready for review Pull request is ready to be reviewed labels Mar 23, 2022
@CarmitKl CarmitKl added ready for review Pull request is ready to be reviewed and removed IN PROGRESS labels Mar 23, 2022
@@ -133,7 +147,12 @@ export const ToastProvider = () => {

return (
<Toaster
className="toast"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this call defined?

@dan-ziv dan-ziv closed this Mar 27, 2022
@dan-ziv dan-ziv deleted the feat/improveResponsiveness branch March 29, 2022 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants