-
Notifications
You must be signed in to change notification settings - Fork 70
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
RWD/my profile and members #4577
RWD/my profile and members #4577
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
About the my profile page:
Since Pioneer doesn't mobile wallet nothing interesting can be done from a mobile in there. Therefore I think we should hide "My profile" from the mobile navigation.
About the members page:
It looks good over all but the role filter part:
packages/ui/src/memberships/modals/BuyMembershipModal/BuyMembershipModal.tsx
Outdated
Show resolved
Hide resolved
packages/ui/src/memberships/modals/InviteMemberModal/InviteMemberModal.tsx
Outdated
Show resolved
Hide resolved
@@ -65,6 +67,8 @@ export const Modal = ({ onClose, modalHeight = 'm', children, modalSize, isDark, | |||
|
|||
useEscape(() => onClose()) | |||
|
|||
if ((screenSize === 'xxs' || screenSize === 'xs') && modalSize !== 'xs') return null |
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.
However I don't think this is the right place to handle this globally. Instead could you do that in the GlobalModals
component please ?
This way some exception can be added for some of the modals. Also instead of just returning null
I think a small modal should be shown with a message like:
This action requires a browser extension, please visit this page from a desktop browser
Finally the condition shouldn't be just be the window size because it would mean that if someone reduce their window for a moment they would lose all the data the input in a modal. As a result I think that:
- it should check this whether a wallet is connected (if it is nothing should be shown).
- instead of checking
screenSize
, the browserscreen.width
should be used here.
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.
I see what you mean, if no wallet is connected so there is no membership, then the banner appears on the top of the screen.
Co-authored-by: Theophile Sandoz <[email protected]>
tested on: https://dao-7pvcye50j-joystream.vercel.app My profileThere is one problem and several remarksProblem: Remarks:
I think this message is needed where we show nothing. For example: Invite a member modal (screen: 320px, 425px), etc. |
@polikosi
This message appears when clicking |
Yes, that's what I mean. |
@eshark9312 Yes, everything is working as expected. Good job, thank you. |
Designs: