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

Fixed SearchBar width constantly changing in Safari #160

Merged
merged 4 commits into from
May 27, 2024

Conversation

Albermonte
Copy link
Member

Too much JS computation, let CSS work its black magic
Tweaked max-width duration value to achieve the same animation effect

Reported error:

Screen.Recording.2024-04-19.at.20.35.17.mov

Fixed:

Screen.Recording.2024-04-20.at.21.10.19.mov

Tested Safari and Brave on macOS. Please test on other OS.

@sisou
Copy link
Member

sisou commented Apr 21, 2024

When testing, test also on mobile widths, and with long content there.

@onmax onmax self-requested a review April 21, 2024 10:22
@onmax
Copy link
Member

onmax commented Apr 21, 2024

Seems fine in chromium for Windows.

Issues

Long content

image

For the long content something like this seems to work:

.cross-close-button {
  right: 1rem;
}

input {
  padding-right: 4rem;
}

Mobile + Staking

image

Maybe hide the staking button and the three dots menu when search bar is focussed?

Maybe too complex CSS 🤔

// src/components/SearchBar.vue - Note that this is another style tag and it is not scoped to the component
<style>
.actions-mobile:has(input.search-bar:focus-within) :is(.reset.icon-button,.prestaking-button) {
   opacity: 0;
}
</style>

@Albermonte
Copy link
Member Author

Albermonte commented Apr 22, 2024

Thanks for the feedback @sisou @onmax

No more issues with long content

Desktop
image

Mobile
image

SearchBar on Mobile takes all space

In small phones there was problems when you have the cashlink button + staking + 3 dots, now everything to the right of the searchbar will be set to display: none when focused

No focus
image

Focused
image

Extra

Also made some improvements on how the placeholder text is calculated (tested and working in Safari)

@Albermonte Albermonte requested a review from sisou April 22, 2024 14:23
@onmax
Copy link
Member

onmax commented Apr 29, 2024

Tiny detail:

There is a pixel shift to the top on mobile when you focus the input on mobiles

Recording.2024-04-29.153218.mp4

(Put attention to the to left of the input to see the bug).

I tried to debug it but not sure where the problem is:

  • It is not the box-shadow
  • It is not the "cross" button

It is a very tiny thing, so not sure if we want to tackle this now :/

onmax

This comment was marked as duplicate.

Copy link
Member

@onmax onmax left a comment

Choose a reason for hiding this comment

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

LGTM

@mraveux
Copy link
Member

mraveux commented May 25, 2024

Looks good to me. Tested it on Safari and it works as expected. Nice 👍

The only things that bothered me was the "feel" from the transitions, which I tried to finetune and I think it feels better now.

Albermonte and others added 4 commits May 27, 2024 16:41
Too much JS computation, let CSS work its black magic
Tweaked max-width duration value to achieve the same animation effect
- now transitioning from and to the real min-width instead of 0, which was adding a delay in the visual transition.
- finetuned transition timings
@sisou sisou force-pushed the albermonte/search-bar-fix branch from 892030a to 9a08d1f Compare May 27, 2024 14:41
@sisou sisou merged commit 0c977e9 into master May 27, 2024
1 check passed
@sisou sisou deleted the albermonte/search-bar-fix branch May 27, 2024 14:42
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.

4 participants