APPSRN-512 Agregar organism ScreenActions a ui-native#75
Hidden character warning
Conversation
Bottom action bar with a rows + flex weights layout model. Two APIs: - actions config: array where a nested array is a row, each action is ButtonProps + optional flex (falsy entries are filtered out) - composition: ActionBar.Row / ActionBar.Item for layouts the config cannot express, without requiring a new ui-native release The bar only owns its chrome (padding, gap, background, bottom safe area via SafeAreaInsetsContext); buttons style themselves. Variants: rounded (default) and flush.
ActionBar exposed two ways to declare the same layout: the `actions` config (nested arrays = rows) and a composition API (ActionBar.Row / ActionBar.Item children). Keeping both meant a bigger surface to document, test and keep in sync for no real gain. We keep only the config: it already expresses every design layout (single CTA, stacked, uneven pair, icon + CTA, row of N, 2x2 grid) via rows + flex weights, and makes the migration from LayoutWithBottomButtons' `buttons` prop almost mechanical. Two APIs for the same job is the kind of optional flexibility that ages into dead code and divergent usage across the apps. Drop Row, Item and the variant context; the config renders its rows and item slots inline. One API, one place to look.
ActionBar read the bottom safe-area inset (SafeAreaInsetsContext) and added it
to its padding, gated by a `withSafeArea` prop. That double-counts the inset in
the real apps.
All three apps (delivery, wms, picking) already wrap the whole tree in a single
<SafeAreaView edges={['top','left','right','bottom']}> at their Main screen, and
their local LayoutWithBottomButtons forks position the bar with plain flex
(margin-top: auto), never reading insets. The SafeAreaView component does not
re-provide SafeAreaInsetsContext with the consumed edge zeroed — descendants
still read the full physical bottom inset — and its default edge mode is
additive. So an ActionBar reading the inset inside that root would push the
buttons up by the inset a second time.
Safe-area ownership belongs to the consumer (the screen/app root), not to a
reusable bar; reading it here only made sense in Storybook, which has no such
root. Drop SafeAreaInsetsContext, the `withSafeArea` prop and the bottomInset
math; the bar keeps uniform chrome padding and matches the forks it replaces.
Adopts the name the team preferred for this component since the original proposal. The structural argument that briefly favored a different name (the container/child plural of the composition API) is moot now that the component is config-only, so this is purely the agreed naming. Renames the folder, component, props/variant types, the public export in src/index.ts and the Storybook story (title included). Behavior unchanged; 14 tests green, tsc + eslint clean.
An icon-only Button collapses to a circle: its intrinsic width (icon + padding ≈ 50) matches its height (50) and the pill radius rounds it fully. In an action bar the icon button should read as a pill next to the main CTA, not a circle. ScreenActions now applies a minWidth to actions that are icon-only (icon and no value), merged with any style the action already carries. The value lives in a named constant (iconButtonMinWidth) so it's a one-line tweak once the exact Figma spec is confirmed. Text buttons are untouched.
Fold the falsy-filter into the row-normalizing map instead of chaining a second map; same result, one pass less.
pablonortiz
requested review from
GonzaFran,
Sebaavt95,
christian97dd,
colomfernando,
dgiannico19 and
pablodamianberdun
Coverage Report for CI Build 28050678588Coverage remained the same at 100.0%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
GonzaFran
left a comment
GonzaFran
left a comment
There was a problem hiding this comment.
@pablonortiz lo veo bien, dejo 2 consultas/ajutes, pero el resto está bien!
| // collapsing into a circle. From Figma: 72px over the 48px button height. | ||
| export const iconButtonMinWidth = scaledForDevice(72, moderateScale); | ||
|
|
||
| export const chromePadding = (variant: ScreenActionsVariant) => |
There was a problem hiding this comment.
@pablonortiz qué significa chromePadding? lo simplificaría a getPadding, dependiendo de a qué te refieras con "chrome"
There was a problem hiding this comment.
Acá el chrome significa el framing alrededor del contenido (toolbars, padding, bordes, etc), acá hay un articulo que explica un poco el concepto: https://www.nngroup.com/articles/browser-and-gui-chrome/. Pero igualmente al no ser tan claro terminé yendo por barPadding, no fui con getPadding porque termina rompiendo un poco el patrón que hay en estas utils, porque como por ejemplo está rowGap(variant) y no es getRowGap(variant), para seguir con esa consistencia fui con barPadding
| import {moderateScale, scaledForDevice} from 'scale'; | ||
| import type {ButtonProps} from 'molecules/Button'; | ||
|
|
||
| export type ScreenActionsVariant = 'rounded' | 'flush'; |
There was a problem hiding this comment.
@pablonortiz una pavada; entiendo que con'flush'te estás refiriendo a que todos los botones estén juntos y que se elimina la separación entre ellos, pero 'rounded' me sugiere que las acciones de la pantalla son botones redondeados, cuando en cualquiera de las 2 variantes el botón de acción está redondeado.
Me parece que, en todo caso, lo que estas haciendo sería más cercano al concepto de 'space-evenly', o 'spaced', que 'rounded'
There was a problem hiding this comment.
Ahí lo ajuste, tiene todo el sentido del mundo lo que decís, me había quedado mal por heredarlo de LayoutWithBottomButtons, lo pasé a spaced que tiene muchísimo más sentido
The 'rounded' variant name was inherited from LayoutWithBottomButtons, where it actually set the button border radius. In ScreenActions the radius is owned by Button, so the variant only controls bar spacing (padding + gap); 'spaced' vs 'flush' describes that axis honestly. chromePadding becomes barPadding to drop the ambiguous 'chrome' jargon while keeping the rowGap-style naming.
| import {moderateScale, scaledForDevice} from 'scale'; | ||
| import type {ButtonProps} from 'molecules/Button'; | ||
|
|
||
| export type ScreenActionsVariant = 'spaced' | 'flush'; |
There was a problem hiding this comment.
@pablonortiz aca estoy viendo definiciones de types que no serian utils. Dado que son pocos, propongo que esten en el archivo del componente directamente
| } | ||
|
|
||
| type SkippableAction = ActionConfig | null | false | undefined; | ||
| export type ActionsRows = Array<SkippableAction | SkippableAction[]>; |
There was a problem hiding this comment.
esto me parecio muy confuso porque leo que una posibilidad que aceptamos es SkippableAction y a simple vista hace ruido. Despues tenes que entender que hace SkippableACtion.
Lo que haria aca seria eliminar esas declaraciones de SkippableAction null, false y demas y declarar que aceptamos
type ActionsRows = Array<Action | Action[]>;
Luego el remplazo. Lo que si hay que mantener es la normalizacion y validacion pero queda mas claro que aceptamos
There was a problem hiding this comment.
Lo que pasa es que el tipo permisivo es a propósito, habilita que podamos pasar botones condicionalmente cómo por ejemplo con
actions={[save, canDelete && deleteAction]}
Esto sin que se tenga que hacer la validación por fuera del componente antes de pasarselo.
Me fijé con tsc --strict: con Array<Action | Action[]> y ese canDelete && deleteAction (que es false | Action) tira TS2322 y nos terminaría obligando a meter ternarios o .filter(Boolean) antes de pasarselo al componente, tal que así:
const filteredActions = [save, canDelete && deleteAction].filter(Boolean) actions={filteredActions}.
Y esto no es un tema de comodidad en un solo lugar o en un caso único, en muchísimos lugares de los tres repos tenemos botones que renderizamos condicionalmente, y no le veo el sentido de agregar un .filter(Boolean) o ternarios en todos estos lados si lo puede manejar el mismo componente sin agregarle lógica excesiva (solo agregar el tipado este que está)
| backgroundColor?: string; | ||
| } | ||
|
|
||
| const itemStyle = (flex = 1) => (flex > 0 ? {flex} : {flexGrow: 0, flexShrink: 0}); |
There was a problem hiding this comment.
aca faltaria importar el type de React native de ViewStyle para tipar
| */ | ||
| const ScreenActions = ({ | ||
| actions, | ||
| variant = 'spaced', |
There was a problem hiding this comment.
@pablonortiz yo creo que eliminaria la opcion de variant para dejar siempre el espacio. ME suena que si no tenes espacio esta mal
…SRN-512-creación-componente-screenactions
- remove the flush variant: it is unused in any design, so the bar always spaces its actions; rowGap and barPadding become plain scaled constants - type the itemStyle return as ViewStyle - move the action types (ActionConfig, ActionsRows) to the component file; utils imports them with a type-only import
….com:janis-commerce/ui-native into APPSRN-512-creación-componente-screenactions
|
@colomfernando ya ajusté tres de los cuatro comentarios que dejaste, el otro no creo que habría que ajustarlo, te lo dejé en la respuesta y cualquier cosa lo vemos |
…SRN-512-creación-componente-screenactions
- JSDoc + Action type TSDoc explain that falsy entries are filtered, so callers can inline conditionals (`cond && action`) like a ReactNode child - drop the dead `variant` argType from the story (variant was removed) - add a ConditionalActions story with a `canCancel` boolean control
….com:janis-commerce/ui-native into APPSRN-512-creación-componente-screenactions
|
@colomfernando listo el PR con ajustes! |
4 participants
Link al ticket
Descripción del requerimiento
Las botoneras inferiores de Orders, Delivery e Inventory se arman con
LayoutWithBottomButtons, que soporta un set fijo de disposiciones de hasta 3 botones. Las disposiciones nuevas de diseño —anchos desiguales, ícono + acción principal, fila doble + botón completo, grilla 2×2— no se pueden expresar con ese set: cada app tiene copias locales adaptadas y las pantallas con layouts nuevos arman la botonera a mano.El impacto es que cada layout nuevo requiere modificar la librería, publicar versión y duplicar el trabajo en las tres apps.
Descripción de la solución
Se agrega el organism
ScreenActionsa ui-native. La API es config-only: la propactionsrecibe un array donde cada elemento plano es una fila de un solo botón, y cada array anidado es una fila de múltiples botones que comparten el ancho por pesos flex. Las entradas falsy se filtran, por lo que los callers pueden inyectar condicionalmente acciones sin ternarios extra.El componente resuelve solo el chrome de la barra: variantes
rounded(padding + gap escalados) yflush(borde a borde sin espaciado) y color de fondo configurable. Los botones se estilizan con sus propias props deButton. Las acciones icon-only reciben unminWidthde 72px (spec de Figma) para renderizarse como pastilla en lugar de círculo.El safe-area inferior no lo maneja el componente: queda en manos del root de la app (un
SafeAreaViewcon el edgebottomactivado). Leerlo en el componente también causaría doble inset en los consumers actuales.LayoutWithBottomButtonsno se toca — queda deprecado, sin breaking changes. El componente nuevo se exporta desde el barrel principal (src/index.ts).Nivel de pruebas requerido
Documentación y ejemplos
¿Cómo se puede probar?
actionscon una acción plana y un array anidado de dos accionesflex: 3yflex: 2icon(sinvalue) junto a una acción principalflex: 0) y tiene al menos 72px de ancho; el principal ocupa el restoactionscon entradasfalse/null/undefinedintercaladasEvidencias, pruebas de cómo funciona
Link a la documentación
CHANGELOG: