Adjust margins and paddings on filter popover

felipepowlist commented 5 months ago

Problem

The popover for filters is already a small component and having it's margins and padding too lose decreases the area to display the actual content. Considering that we can have filters with long lists of items, search and action buttons on the bottom, I believe there's room to balance the white space to focus more on the content and optimizing the usage of space.

Solution

https://www.figma.com/file/n11HvLuYnfhb59w0vAjPWn/Merchant-Org---Ideas-and-Explorations?type=design&node-id=467-67036&mode=design&t=JwcRAORroBksrtje-4

Usage examples

Dependencies

No response

References

No response

matheusps commented 5 months ago

You proposal looks pretty great IMO! Thoughts @pedroalexandria , @beatrizmilhomem ?

beatrizmilhomem commented 4 months ago

Hey, @felipepowlist! Sorry for taking so long to answer, I was on vacation. Great proposal, I can see the benefit of changing the space tokens.

What do you all think of using space-4 instead of space-3? It's just a 4px difference, but with space-3 I think the content is too tight.
@matheusps we used those tokens because of the content container we were using (we have specific padding according to the size of the container). By making changes we would be ignoring that logic. What do you think about that? @anitavincent had already mentioned issues with the usage of content inside a popover https://github.com/vtex/shoreline/issues/1338
We currently use radius-3 only for modals. Should we apply it for all popovers too? If so, we should test in other components.
@felipepowlist I noticed you also changed the text styles in the filter trigger, I also think it's a nice change to the component, I would just maintain using the text-emphasis on the filter name and use text-body on the option applied. What do you think?

Filter

Example of applying radius-3 to the Menu popover. In this case, the space-3 padding works well because the items have an extra padding.

Menu

felipepowlist commented 4 months ago

@beatrizmilhomem I'm aligned with all your proposals! For me, we can move forward with this!

beatrizmilhomem commented 3 months ago

@felipepowlist Hello! As I've mentioned in the comment above, the popover uses the Content component as a base. The Content basically creates rules for padding according to the container width, which is part of the spacing guidelines for the Admin. You can see how the rules work in this Figma component.

Since the popover uses those rules, the changes should happen in the rules and the Content itself, instead of only in this specific popover. Besides Popovers, Content is also currently used in Modals and Pages. So it is necessary to make tests to answer questions like: Should we change only the padding for 240 - 379px content? (Filter's popover is in this width interval) If so, how does it affect other popovers within the same width interval? Will this single change make sense within the spacing usage in the Admin? Or should we review all the Content padding rules, which impact other components?

Considering all that, are you available to make those tests and evolve your initial proposal? It is not a problem if you don't have the bandwidth now! If that is the case, we will tag this issue as design: wanted so that another person (or even you in the future!) can move on with it. If you have any questions, please reach out to any of the maintainers!

felipepowlist commented 3 months ago

I can make this exploration and share here ;)

davicostalf commented 1 month ago

@felipepowlist Have you managed to take a look at this? Do you need any help?

felipepowlist commented 1 week ago

I finally had some spare time to work on this. It's not super structured but I have a proposal for review here

@davicostalf @beatrizmilhomem

beatrizmilhomem commented 1 week ago

Boa @felipepowlist ! ✨

Radius: em geral, tá funcionando pra mim. A nova proposta de uso do radius, pelo que entendi, seria então:

Token	Usage
$radius-2	Buttons, Inputs, Filters, and Tooltips
$radius-3	Modals, Alerts, Toasts, Cards, and Popovers

É isso?

Content: Também achei que tá funcionando bem, mas ainda vale testar: 1) o Page em mobile com conteúdo como vc comentou, incluindo elementos no header e 2) o ContextualHelp usando esse content com conteúdo textual.

davicostalf commented 5 days ago

Achei que as mudanças de padding no content fazem sentido.

Vale separar a mudança de border radius em uma issue separada. Será que não vale um token novo para containers maiores como Modais terem um border radius ainda maior?

felipepowlist commented 3 days ago

Então, eu tenho pensado no radius de forma que eles escalem considerando o que está contido neles. Porém, eu acho que a forma que a @beatrizmilhomem articulou é suficiente pra comunicar essa mudança agora. Sugiro subir esse fix agora e acompanhar pra ver se precisaremos de outros tokens de radius ou se esses serão suficientes.

Fiz testes aqui com conteúdo no mobile e queria saber as impressões de vocês pra gente poder seguir.

davicostalf commented 2 days ago

Estou de acordo. O único empecilho que vejo é que 16px pra mobile não está de acordo com o grid que definimos em platform foundations. Mas 24px também não estava, então acho que podemos seguir em frente.

beatrizmilhomem commented 2 days ago

De acordo também! Bora seguir.

Próximos passos Content

Limpar os testes da branch para que possa ser mergeada
Criar uma nova issue de design listando os componentes que precisarão ser ajustados na lib de Components
Criar uma nova issue de dev para fazer os ajustes no Content em código

Radius

Criar uma nova issue retomando a proposta e listando quais os componentes que precisarão ser ajustados na lib de Components
Depois de fazer os ajustes no Figma, criar uma nova issue de dev para cada componente que precisa ser ajustado em código
Criar uma issue de documentação para atualizar a tabela de boas práticas do Radius

vtex / shoreline