mirror of
https://github.com/mattermost/mattermost.git
synced 2026-11-05 05:37:13 -06:00
79e7448565
* Lay dialog elements out as a grid, and let a dialog pick its width
An Interactive Dialog could already group elements under a heading with a
collapsible container, but it could only stack them vertically. A form
asking for the same handful of fields across several records — a status
per service, a quantity per line item — therefore read as one labelled
block per record, repeating every label, with nothing lining up.
Two changes, both additive.
A grid layout mode
------------------
Setting SubType "grid" on a collapsible lays its children out as a table
rather than stacking them: each child container becomes a row, and that
row's children become its cells. This is a layout over ordinary elements,
not a new element type, which is what keeps it cheap:
- a cell is a real element, so Type, SubType, Default, Optional and
MaxLength all work per cell with no new plumbing
- a cell's value arrives in the submission as an ordinary top-level
field, because collapsible children already flatten that way
- an error returned for a cell lands under that one cell, because the
Errors map is already keyed by element name
So neither the submission model nor the error model changes. The webapp
carries the subtype into CollapsibleConfig.layout, and AppsFormGrid
renders the subtree.
The layout rules worth knowing: column headers come from the first row, a
short row is padded on the right rather than shifted, the first element
of each row is a sticky, still-editable row header, and each cell's label
is hidden visually but kept for assistive technology. A child of the
container that is not itself a container cannot be a row, so it renders
under the table rather than being dropped.
A width tier on the dialog
--------------------------
A grid rarely fits in 600px, which is what every dialog has been. Nothing
ever enforced that width: the apps form renders a react-bootstrap Modal
with no size prop, so it inherited Bootstrap's default, and
dialogClassName was hardcoded to classes that set no width at all.
Dialog.Size takes small, medium or large. Empty means small, so no
existing dialog changes — and an unset size adds no class at all rather
than an explicit small one, so the rendered markup is also unchanged.
Tiers rather than pixels keep it themeable and stop an integration asking
for 2000px; .modal-dialog's max-width still applies, so every tier
shrinks on a narrow viewport instead of overflowing. medium reuses the
existing modal-lg rather than widening it, because Mattermost
deliberately narrows Bootstrap's 900px modal-lg to 700px and other
consumers depend on that.
Width sits on the dialog rather than in server config because the
integration is the party that knows how much content the form holds; one
global width cannot suit both a two-field confirmation and a nine-column
grid. Because a refresh response replaces the contents of the open
dialog, returning a different size resizes it in place.
Tests and docs
--------------
Unit tests cover the grid's layout rules, the subtype-to-layout mapping
(including an unrecognised subtype falling back to stacked), size
surviving each hop from the server response to the modal class, and
Dialog.Size validation including that size is omitted from the JSON when
unset.
The Cypress spec drives the webhook server, which gains a grid dialog in
its own module. It covers the table structure, the editable sticky row
header, adding a row in place with typed values preserved, an error
landing on the one offending cell, the width tier resizing the open modal
without losing input, and a button cell opening a child dialog for its
row.
Collapsible elements were never documented, and the grid is a layout mode
on that container, so both are documented here, along with dialog width.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Keep the grid spec's weight cell on whole numbers
MM-T5700D typed 12.5 into the weight cell and asserted it survived the
refresh. It did not: the assertion got 12, and the spec was the one
failing unit in cypress-full-enterprise.
The grid is not at fault. The weight cell is subtype "number", which
renders an <input type="number">, and TextSetting reports those through
parseInt rather than parseFloat:
props.onChange(props.id, parseInt(event.target.value, 10));
so 12.5 becomes 12 before it reaches the submission at all. The webhook
server folds values back with String(...).trim() and coerces nothing, so
the fraction was already gone by the time the state round tripped. That
parseInt predates this branch by two years and lives in a file this
branch does not touch, so fixing it belongs in its own change: it would
alter the value type every number setting in the app emits, the system
console among them.
The test's subject is that half-finished input survives a refresh, and a
whole number demonstrates that just as well. The weight placeholder drops
from "0.0" to "0" for the same reason, since advertising decimals the
field discards is worse than not offering them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Stop the grid dropping cells, and tighten two loose edges
Three fixes from a review pass over the grid, none of which change how a
well-formed grid renders.
A row longer than the first kept its cells
------------------------------------------
Column count came from the first row, and the body iterated those columns,
so a cell beyond that width was never rendered. That was deliberate and
tested, but the consequence was not: flattenFields recurses into every
collapsible child regardless of layout, and the submit check runs over all
of them, so a required cell in an over-long row was validated while having
no input on screen. The result was a dialog that would not submit and could
not say why, because the error had nowhere to render.
The table is now as wide as the widest row. A heading for a widened column
comes from the first row that has a cell in that position, and a short row
is still padded on the right rather than shifted. For a grid whose rows all
match, which is what the docs ask for, this is exactly the old behaviour.
Keying the headers on the cell name rather than the label also stops two
columns that share a label from colliding, and drops the two array-index
key warnings the file used to produce.
The width transition applied to nearly every modal
--------------------------------------------------
The resize easing was scoped to .modal-dialog.a11y__modal, a class carried
by about a hundred modals across the app, so all of them picked up a width
transition from a form they have nothing to do with. The dialog now carries
an apps-form-modal class and the transition hangs off that instead.
An unknown collapsible subtype passed the server
------------------------------------------------
Dialog.Size rejects anything outside its three tiers, but a collapsible's
SubType was not checked at all, so "gird" validated cleanly, reached the
client and silently rendered as stacked. validateCollapsible now rejects
any subtype other than "grid". It lives there rather than in isValid's type
switch because nesting recurses into validateCollapsible directly to keep
depth tracking and never passes back through that switch, so a nested
container would otherwise be missed.
This is safe to tighten: collapsible appears only in v12.0.0-rc1 and rc2
and has never been in a GA release, so no shipped integration can be
relying on a subtype that was previously ignored.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Give the dialog width section a sequential heading level
The version note under "Dialog width" was an h5 directly beneath an h2,
a three-level jump that trips markdownlint's heading-increment rule and
gives the page a heading outline with two levels missing from it.
It is now an h3, which is the correct next level.
The file has four other heading jumps of the same shape, all under the
11.1 sections and all predating this branch, so they are left alone.
That does mean this section's version note now renders one size larger
than the equivalent note in the neighbouring "Multi-step dialogs" and
"Dynamic field refresh" sections, which are still h5 under an h2.
Making the whole file consistent is a docs change in its own right
rather than something to smuggle into a feature branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Address review: keep the slide, keep the section, say what the server does
Every apps form lost its slide-in animation. Bootstrap's `transition:
transform 0.3s ease-out` sits on `.modal.fade .modal-dialog`, exactly as
specific as the new `.modal .modal-dialog.apps-form-modal` rule and earlier
in the cascade, so naming only `width` replaced it rather than adding to it
— and every apps form carries the class, not just a grid. Both transitions
are listed now.
The three width tiers were the same width between 768px and 991px, because
`medium` borrowed `.modal-lg`, whose 700px Mattermost sets app-wide only
above 992px. Both wide tiers get a class of their own and start at
Bootstrap's own 768px breakpoint, so a dialog's width no longer moves when
an unrelated rule does, and the tiers differ wherever a dialog is not
already fluid.
A grid container was rendering its own subtree, which dropped the three
things a collapsible says about itself: `collapsed`, `borderless` and
`display_name` were all ignored, including in the docs' own grid example.
The grid now lays out inside the same CollapsibleSection the stacked layout
uses, so only the arrangement of the children differs.
Accessibility and ragged input, in the grid itself:
- the table takes the container's name through a clipped `<caption>`, and
each row keeps its `display_name` as the row's accessible name
- a required cell's marker was only in the clipped per-cell label, so the
cell looked optional until submission failed; the column heading carries
it too
- rows that are all empty fell through to an empty table instead of the
fallback
- two cells sharing a name in one row collided on their React key and cost
the row a cell; the keys carry the column index
- the sticky first column could cover the cell that just took focus while
tabbing sideways (`scroll-padding-inline-start`)
The docs and a Go comment said an unrecognised `size` or collapsible
subtype was rejected when the dialog opens. It is not:
App.OpenInteractiveDialog logs what IsValid returns and opens the dialog
anyway, and the client falls back to the default width and to a stacked
layout. Both now say that, and tests pin it on the open-dialog path rather
than only on Dialog and DialogElement in isolation. `12.0` in the two new
sections is `12.1`: v12.0.0 is tagged and has neither feature.
Cells are matched to columns by position, so a row that omits an element in
the middle shifts the rest left under the wrong heading. The docs claimed a
mismatch always showed as a gap; they now say what actually happens and
what to do about it.
Also: the grid spec measured nothing it claimed to — `overflow-x: auto` is
set unconditionally, and the width check only read a class name. It now
measures the modal at each tier, checks the sticky column by scrolling the
table, checks the clip on the per-cell labels rather than `not.be.visible`
(which passes for a clipped element either way), waits on
`cy.intercept`/`cy.wait` like action_button_spec rather than on fixed
delays, and covers a window too narrow for the tier. The MM-T5700A–H IDs
were invented and are gone; 942 specs in the repo carry no TestRail ID.
Jest gains the gaps: required cells and per-cell errors, a collapsed grid,
a grid nested in a grid through AppsForm, unique keys, an unknown size, and
the router and adapter hops that carry `size`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Tell the truth about a required cell and about what a refresh keeps
Two from CodeRabbit, both real.
A column heading carried the required marker of whichever row happened to
supply the heading. If the first row's cell was optional and a later row's was
not, the heading stayed unmarked and the later cell looked optional, because
its own label is clipped. A heading speaks for the whole column, so it is now
marked only when every cell in that column is required; when rows disagree the
marker moves to the cells that actually carry it, hidden from assistive
technology since each cell's clipped label already says so.
The dialog width section promised that "values the user has already entered are
preserved as usual" across a refresh. getDerivedStateFromProps merges the
replacement form's initial values over the current ones, so a field that comes
back without its value echoed as `default` is cleared. The grid section already
said to echo them; the width section now says so too.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Point the sticky test at a window the grid actually overflows
Both of the spec's scroll assertions were false, which is how a real assertion
differs from the `overflow-x: auto` check it replaced: that one passed because
the stylesheet sets it unconditionally.
The sample grid is four columns and squeezes to about 500px. Every width tier
on a desktop window leaves more room than that -- 510px inside the 600px tier,
610 inside 700, 810 inside 900 -- so the table always fits and never scrolls.
The sticky test now narrows the window below that floor, where there is
genuinely something for the row header to stay put against, and the width test
no longer claims the grid fails to fit at the narrow tier, because it does fit.
Verified against a local server built from this branch: 9 passing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Say what a refresh actually does to an unechoed field
Not every field type is cleared: initFormValues falls back to `field.value ||
defaultValue`, which is empty for an ordinary field but `false` for a `bool`
and the current time for a required `datetime`. "Will clear" overstated it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Drop the blank line left behind by the removed assertion
Taking the overflow assertion out of the width test left its block padded,
which @stylistic/padded-blocks rejects.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Re-run CI
Empty commit. playwright-full failed on b437b625 with four tests that each
passed and failed against this same code in the same run, so this is to see
them run again rather than to change anything.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Give the apps form dialog its own width class
The width tiers put apps-form-modal on the modal's dialogClassName, but
that class is already on the outer .modal, so every open dialog matched
.apps-form-modal twice and the stacked-dialog E2E specs counted double.
Hang the width styles off apps-form-modal-dialog instead.
The grid button-cell spec also counted #appsModal and looked up the
parent's #appsModalLabel, neither of which a hidden parent dialog has.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---------
Co-authored-by: Jack Benning <jack.benning@mattermost.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Jack Benning <jack@Jacks-MacBook-Pro.local>