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

ParaTime picker #426

Merged
merged 20 commits into from May 26, 2023
Merged

ParaTime picker #426

merged 20 commits into from May 26, 2023

Conversation

buberdds
Copy link
Contributor

@buberdds buberdds commented May 23, 2023

PR covers only desktop version

Need to go through this with Don tomorrow + there are some missing descriptions in lang files. This is done

@github-actions
Copy link

github-actions bot commented May 23, 2023

Deployed to Cloudflare Pages

Latest commit: 8241cb32ccb9c57cc72ea3b4e5711a3d343d2faf
Status:✅ Deploy successful!
Preview URL: https://ee1643bc.oasis-explorer.pages.dev

@buberdds buberdds force-pushed the mz/picker branch 8 times, most recently from 0f7eb7f to 2919f0f Compare May 24, 2023 15:40
src/app/components/ParaTimePicker/ParaTimeMenu.tsx Outdated Show resolved Hide resolved
Comment on lines 38 to 57
onMouseEnter={() => {
if (layer !== selectedLayer) {
setSelectedLayer(undefined)
}
setHoveredLayer(layer)
}}
Copy link
Member

Choose a reason for hiding this comment

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

I get annoyed with this UX:
ux

src/app/components/ParaTimePicker/index.tsx Outdated Show resolved Hide resolved
src/app/components/ParaTimePicker/index.tsx Outdated Show resolved Hide resolved
@buberdds buberdds force-pushed the mz/picker branch 6 times, most recently from 97009b9 to 45f4daf Compare May 25, 2023 12:10
@buberdds
Copy link
Contributor Author

Ok, I've finished working on design feedback with Don. Some issues added by @lukaw3d are resolved. In terms of small screen I guess we will switch to mobile version once it's ready.

@csillag
Copy link
Contributor

csillag commented May 25, 2023

I checked this out, and have a few difficulties with with, and suggestions.

  1. By default, we only see the level where we can select Paratimes within the network. There is no indication whatsoever that it should be possible to switch between networks. (Even when I am running in TestNet mode!) There is only this little arrow at the top, which is nor clear at all. I think at least a work of explanation would be very useful there.
  2. When I click that semi-hidden button, the network selector is revealed, but it only shows the Mainnet, even if I am currently at TestNet! I understand that we don't want to promote TestNet too much, but when we are already on TestNet, what is the point of hiding it's existence? I think it's confusing that the opening state is not synced to the current setting. I think if we are at TestNet, it should shot TestNet selected by default.
  3. Why is the currently selected state not highlighted by a different bg color? (Both network and Paratime.)

@donouwens I'm sorry to bring the feedback so late in the game, but I think it's better late then never.

@donouwens
Copy link
Collaborator

Thanks for the input @csillag

  1. True, but we don't want to promote the Testnet (and Devnet) to prominently at this stage, which is why we've decided to have it collapsed. Once/if we get to the point where we do want users to note the networks easier we'd simply not have the collapsed state anymore.

  2. This is true - if the user is one Testnet we should show the Testnet icon or Testnet icon with 'Testnet' title.

  3. The selected state should show the content in our dark brand (#3333C4) along with the chevron pointing to the right. The reason for not having a different background color is to avoid confusion with the hover state.

@csillag
Copy link
Contributor

csillag commented May 25, 2023

  • For point 1: Having it collapsed is OK. All I am advocating for is having a single word of explanation besides the arrow.
  • For point 3: For me, on my monitor, the difference between the selected and not selected states is undetectable. About confusion with the hover state: the hover state has both the moving arrow and the background color change. I think we could take one of the two to indicate the selected state. (Or something else, but the current way I just can not see.)

@donouwens
Copy link
Collaborator

  • For point 1: Having it collapsed is OK. All I am advocating for is having a single word of explanation besides the arrow.

Having a word next to it will make it look like part of the Paratimes rather than the arrow due to the juxtaposition. I'm confident our users are curious enough to toggle that arrow, but it's something we can keep an eye on in future user testing.

  • For point 3: For me, on my monitor, the difference between the selected and not selected states is undetectable. About confusion with the hover state: the hover state has both the moving arrow and the background color change. I think we could take one of the two to indicate the selected state. (Or something else, but the current way I just can not see.)

My bad I replied without double checking - the active state does have a background in the designs. Has it not been implemented correctly? I feel there are significant color differences and visual indicators between default/selected/hover/disabled (which also has the tooltip) - see screenshot.

Screenshot 2023-05-25 at 21 26 43

src/app/components/ParaTimePicker/LayerMenu.tsx Outdated Show resolved Hide resolved
src/app/utils/layout.tsx Outdated Show resolved Hide resolved
src/locales/en/translation.json Outdated Show resolved Hide resolved
@csillag
Copy link
Contributor

csillag commented May 25, 2023

the active state does have a background in the designs. Has it not been implemented correctly? I feel there are significant color differences and visual indicators between default/selected/hover/disabled (which also has the tooltip) - see screenshot.

Screenshot 2023-05-25 at 21 26 43

No, it doesn't look that way. This is the current implementation:

So,

  • In the design, the lighter shade of highlight signifies the current selection, and the hover is a stronger shade of highlight.
  • However, in the implementation, there is only one shade of highlight, and it is used by hover, and the selection only has the small "(currently active)" flag.

Should be easy to fix, though.

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.

Please make it so that the selected state is highlighted, like it is in the design. Maybe my eyes 👀 are not what they used to be, but I can hardly see which one is selected..

@csillag
Copy link
Contributor

csillag commented May 25, 2023

When I click that semi-hidden button, the network selector is revealed, but it only shows the Mainnet, even if I am currently at TestNet! I understand that we don't want to promote TestNet too much, but when we are already on TestNet, what is the point of hiding it's existence? I think it's confusing that the opening state is not synced to the current setting. I think if we are at TestNet, it should shot TestNet selected by default.

This is true - if the user is one Testnet we should show the Testnet icon or Testnet icon with 'Testnet' title.

That part works, we can see the Testnet icon and title, that's not what I'm talking about.
My issue is that when I bring out the network selector, it only shows Mainnet, and hides Testnet, which is the current selection.

image

So, to summarize: we are on Testnet, why is testnet not listed as one of the options, and marked as selected?

@buberdds buberdds force-pushed the mz/picker branch 2 times, most recently from d1ee717 to 3ee730d Compare May 26, 2023 10:16
@csillag
Copy link
Contributor

csillag commented May 26, 2023

OK, can clear up the dictionary? I am suspecting that we might assing different meaning to some of the terms here.
The state I'm talking about (selection type 2) is definitely not a split second state; it starts when the users clicks on the list item, and exists indefinitely, at least until the user clicks the "select" button at the other side of the dialog, hence closing the dialog.

@buberdds
Copy link
Contributor Author

imo it doesn't make sense to discuss such design changes via code review process. It's better to discuss this during Sync meeting. I will keep PR open until then.

@csillag
Copy link
Contributor

csillag commented May 26, 2023

Nah, lets just merge it. it has already improvr
ed a lot.

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.

LGTM

@buberdds buberdds merged commit 8db597a into master May 26, 2023
4 checks passed
@buberdds buberdds deleted the mz/picker branch May 26, 2023 13:34
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

4 participants