Page MenuHomePhabricator

Problem: the ooui dialog has a padding that we want/need _inside_ of our app, not around it
Closed, ResolvedPublic

Description

The OOUI Dialog modal we use for our app comes with padding. While this was initially appreciated, it now turns out that this padding is problematic as we do not want that padding around our header component (T230328, T230329), but inside our "body".

There seem to be some possibilities to remove that misplaced padding:

  • add a class to the modal via the dialogs classes setting and override the defiinition of that padding with padding: 0 (recommended)
  • use negative margins on our app to effectively undo the padding ( hacky and probably brittle )
  • use a OOUI ProcessDialog instead of a Dialog and reuse the OOUI buttons, probably not as responsive as we need it

Event Timeline

Michael triaged this task as High priority.Aug 21 2019, 8:04 AM
Michael updated the task description. (Show Details)

Change 532729 had a related patch set uploaded (by Pablo Grass (WMDE); owner: Pablo Grass (WMDE)):
[mediawiki/extensions/Wikibase@master] bridge: use padding-less panel for bridge dialog

https://gerrit.wikimedia.org/r/532729

Michael moved this task from Doing to Peer Review on the Wikidata-Bridge-Sprint-4 board.
Michael added a subscriber: Pablo-WMDE.

I adjusted the variables in _variables.scss in two ways:

  1. I renamed $base-font-size-desired to $container-relative-font-size as that is what it is and that makes its purpose in the calculations clear: to undo the change in font-size coming from container by dividing by $container-relative-font-size
  2. I kept the explizit px units for the calculation as I find them to helpful in understanding the calculations. I assume they are not in the common.less because Less handles units in calculations strangely. (This is an update from my previous position that it is better to remove thos px units.)

Let me know what you think!

I looked at the patch as it evolved and tried to rebase it. In the mean time changes by the same author were merged (for T230328) into the main line which take a different approach to a somewhat similar challenge so I don't quite know which is the desired one...

So maybe first things first: do we agree that at default base font size (16px), the size of the padding should be a computed value of 16px?
We probably don't want to express this in px but use a relative unit instead. Do we consequently want this padding to be relative to the font-size of this element (arguably an inherited one) or the base font size? In my suggestion I went with the first option because that's what happens in OOUI (1.14285714em, not rem).
Based on that decision we can talk about different ways to reach that goals and possible functions to made the code more convenient/elegant (e.g. a calculate-em() function).

A side note: I don't see much value in putting all variables in the _variables file, especially so if they are only used once and carry overly specific names (effectively ruling out even future reuse).