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.

Sign up for GitHub

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

Jump to bottom

Mobile ParaTime picker #482

Merged
merged 8 commits into from Jun 19, 2023
Merged

Mobile ParaTime picker #482

merged 8 commits into from Jun 19, 2023

Conversation

lubej
Copy link
Collaborator

@lubej lubej commented Jun 8, 2023

No description provided.

@lubej lubej force-pushed the ml/mobile-paratime-picker branch from 4c63ffd to 6840586 Compare June 8, 2023 16:56
@github-actions
Copy link

github-actions bot commented Jun 8, 2023 •
edited

Deployed to Cloudflare Pages

Latest commit: 73a0ca60a1ceb787ac9be04fe3d4051d9734c8e7
Status:✅ Deploy successful!
Preview URL: https://bcbe8638.oasis-explorer.pages.dev

@lubej lubej force-pushed the ml/mobile-paratime-picker branch 4 times, most recently from b1b4fbd to fe5a22f Compare June 9, 2023 15:08
@lubej lubej marked this pull request as ready for review June 9, 2023 15:11
@lubej lubej requested review from csillag and lukaw3d June 9, 2023 15:11
@csillag csillag mentioned this pull request Jun 12, 2023
Merged
@csillag
Copy link
Contributor

csillag commented Jun 13, 2023 •
edited

A few questions / suggestions, @donouwens please comment.

  1. Currently, when opening the paratime picker on mobile, we start with the network selection page. I don't think we want that. The idea was that we don't really want to promote this whole idea of having different network, as of yet. That is why on the desktop, when opening the paratime picker, the network selection menu is hidden; it has to be opened using a separate button. Therefore, I think that when opening it on mobile, we should start with the layer selector, and it should require a dedicated action to get to the nework selection. Right?

  2. Shall we us Paratime or ParaTime ? I think we should standardise on one, but now I see both versions.

  3. After selecting a paratime from the list, we arrive to the confirmation screeen. The buttons at the bottom say cancel / select, and the way to get back to the list is on the top, saying "Select ParaTime". My problem is that we are using "select" twice, with different meaning here. On the top, "select Paratimes" means "go back to the list", and on the bottom, "select" means "confirm my selection and close the dialog". I think we should change the wording to be less confusing. Maybe say "Back to the list of Paratimes" on the top?

@csillag
Copy link
Contributor

csillag commented Jun 13, 2023

#500 has been merged, also touching the same code, so unfortunately this will need to be rebased now, @lubej . Sorry about that.

@donouwens
Copy link
Collaborator

  1. Yes, but the idea was that when the ParaTime picker is opened on mobile we initially show the selected ParaTime the user is on (so this view: https://www.figma.com/file/ifCrok8cP5ymEYjMa2PIi9/MVP?type=design&node-id=4629%3A220644&t=1uafovw9TQaRA1lp-1). This way the user has direct access to the info for the ParaTime they are on.

  2. I recall agreeing on ParaTime, but I don't have a strong opinion on this (other than that I agree with @csillag that we should unify it).

  3. Good point. I think your suggestion is good, but too long @csillag - perhaps we can simply say 'View Networks' and 'View ParaTimes'?

@lubej lubej force-pushed the ml/mobile-paratime-picker branch from fe5a22f to df5b260 Compare June 13, 2023 12:18
@lubej lubej requested a review from buberdds June 13, 2023 12:23
@csillag
Copy link
Contributor

csillag commented Jun 13, 2023

Tested this again. Two comments/questions, to @donouwens and then to @lubej :

  1. On the desktop version, the button opening the network menu is nameless; there is no text, in order to de-emphasize it. However, in the mobile version, it ways "< View Networks". I think if we want to be consistent, we should also hide the text here, and just go with the "<", right?
  2. For the disabled layers (like Consensus, Cipher, and even Emerald but it's not yet visible on this branch), there is no indicator whatsoever. On the desktop we have at least the tooltip, but on mobile there is no hover so no tooltip. There should be something, right? I think a design has a label saying "coming soon" or something like that....

@donouwens
Copy link
Collaborator

  1. I'd like to keep it as it is for now as on desktop we show a minimum of two columns which gives the user an indication of further info (and less emphasis). On mobile though, the user only sees one column at a time so there's a bigger need to communicate what the '<' does.

  2. Correct, that's what the visuals have as well: https://www.figma.com/file/ifCrok8cP5ymEYjMa2PIi9/MVP?type=design&node-id=4629%3A221798&t=yxNwHmEP5a0zw9FC-1

@lubej lubej force-pushed the ml/mobile-paratime-picker branch 5 times, most recently from aacdf12 to f334f45 Compare June 19, 2023 08:07
csillag
csillag reviewed Jun 19, 2023
View reviewed changes
src/app/components/ParaTimePicker/LayerDetails.tsx Outdated
@@ -80,6 +81,7 @@ const contentMinHeight = '270px'
export const LayerDetails: FC<LayerDetailsProps> = ({ network, selectedLayer }) => {
const { t } = useTranslation()
const theme = useTheme()
const isTablet = useMediaQuery(theme.breakpoints.down('md'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use the newly available useScreenSize() for this? It already provides isTable. I think it would be confusing to use different definitions at multiple points of the code... right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, leftover from the rebase. Nice catch 👍

@csillag
Copy link
Contributor

csillag commented Jun 19, 2023

Reviewed the code, generally looks good (albeit there is lots of ugly CSS magic). I have left a bunch of small comments, and I have a bigger question which we will discuss in person

@lubej lubej requested a review from csillag June 19, 2023 12:42
@lubej lubej force-pushed the ml/mobile-paratime-picker branch from c35ad3a to 73a0ca6 Compare June 19, 2023 12:43
csillag
csillag approved these changes Jun 19, 2023
View reviewed changes
Copy link
Contributor

@csillag csillag left a comment

Choose a reason for hiding this comment

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

Reviewed. LGTM.

@lubej lubej merged commit 5525118 into master Jun 19, 2023
7 checks passed
@lubej lubej deleted the ml/mobile-paratime-picker branch June 19, 2023 12:53
@buberdds
Copy link
Contributor

on https://oasis-explorer.pages.dev/ it looks a bit weird
Screenshot 2023-06-19 at 16 50 51

@lubej
Copy link
Collaborator Author

lubej commented Jun 19, 2023

on https://oasis-explorer.pages.dev/ it looks a bit weird Screenshot 2023-06-19 at 16 50 51

Ah right, the warning message hides when scrolled to the bottom. Will fix

@lubej lubej mentioned this pull request Jun 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csillag csillag csillag approved these changes

@lukaw3d lukaw3d Awaiting requested review from lukaw3d lukaw3d is a code owner

@buberdds buberdds Awaiting requested review from buberdds buberdds is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lubej @csillag @donouwens @buberdds