Skip to content

Adjust viewport height for mobile devices on Update Shelter page #232

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

Merged

Conversation

felipemrvieira
Copy link
Contributor

Copy link
Member

@lucianomlima lucianomlima left a comment

Choose a reason for hiding this comment

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

Coloca uns prints mostrando que resolveu e principalmente que não impactou outras resoluções.

@felipemrvieira
Copy link
Contributor Author

192 168 0 12_5173_abrigo_3a5e8ab5-5c48-48f3-98f1-f7c9c5b26ddb_atualizar(Pixel 7)
192 168 0 12_5173_abrigo_3a5e8ab5-5c48-48f3-98f1-f7c9c5b26ddb_atualizar(iPhone 14 Pro Max)
Screenshot from 2024-05-17 13-14-55
pocox3

dadedeandrade
dadedeandrade approved these changes May 17, 2024
dadedeandrade

This comment was marked as off-topic.

Copy link

@thiagomrvieira thiagomrvieira left a comment

Choose a reason for hiding this comment

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

Lgtm

@allangalera
Copy link

Postei na issue original. Acredito que podemos resolver isso usando apenas css -> https://caniuse.com/viewport-unit-variants

Copy link
Contributor

@vinny-silveira vinny-silveira left a comment

Choose a reason for hiding this comment

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

Analisando bem... Não precisaria nem mesmo o useEffect com os listeners, apenas o cálculo do "vh custom" no topo do arquivo fora da "declaração do componente", pois na prática, o usuário não vai redimensionar a página no smartphone. Fiz alguns testes locais aqui e funcionaram normalmente (desktop e mobile), você pode alterar e fazer mais uns testes locais também, quanto menos a necessidade de useEffect melhor., resolvido dessa forma mesmo com Tailwind:

document.documentElement.style.setProperty('--vh', `${window.innerHeight * 0.01}px`);

const UpdateShelter = () => {
// ...

  return (
    <div className="flex flex-col items-center h-[calc(var(--vh,1vh)*100)] md:h-screen">

// ...

Para telas maiores que 768 (md), pega o vh completo, abaixo disso, ele calcula.

@vinny-silveira
Copy link
Contributor

Postei na issue original. Acredito que podemos resolver isso usando apenas css -> https://caniuse.com/viewport-unit-variants

Bem mais simples mesmo essa abordagem @allangalera, 92.34% dos browsers e versões já suportam essa unidade:

image

@felipemrvieira, como temos um suporte para mais de 90%, considere o uso do dvh por gentileza.

@felipemrvieira felipemrvieira force-pushed the fix/viewport-height-mobile branch from f994ed5 to dc949f0 Compare May 22, 2024 19:34
@felipemrvieira
Copy link
Contributor Author

Postei na issue original. Acredito que podemos resolver isso usando apenas css -> https://caniuse.com/viewport-unit-variants

Bem mais simples mesmo essa abordagem @allangalera, 92.34% dos browsers e versões já suportam essa unidade:

image

@felipemrvieira, como temos um suporte para mais de 90%, considere o uso do dvh por gentileza.

Acabei de atualizar o PR. Valeu pelo feedback!

Copy link
Contributor

@vinny-silveira vinny-silveira left a comment

Choose a reason for hiding this comment

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

Por essa abordagem tem uma nível de compatibilidade bem maior que o dvh, uma vez que ele calcula a propriedade apenas no "primeiro load" da aplicação e usa a unidade px, PR apto ao merge.

@vinny-silveira
Copy link
Contributor

@SOS-RS/frontenders Preciso de mais um review aqui.

Copy link
Contributor

@dadedeandrade dadedeandrade left a comment

Choose a reason for hiding this comment

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

Perfeitooo

@vinny-silveira vinny-silveira requested a review from a team May 24, 2024 16:53
@felipemrvieira
Copy link
Contributor Author

Acredito que está pronto para merge @vinny-silveira @andradeviniicius @larissapissurno

@larissapissurno larissapissurno merged commit 37cc2ec into SOS-RS:develop Jun 2, 2024
@larissapissurno
Copy link
Contributor

mergeado @felipemrvieira

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.

[BUG] Botão "atualizar" de atualizar abrigo aparece cortado para fora da tela - mobile
8 participants