Add Divider - #92
Conversation
MaxTCodes
left a comment
There was a problem hiding this comment.
The Divider follows the existing component and registration patterns, and the tests cover its main creation and reveal behavior.
I found two issues in Divider:Set that should be fixed before merge. It does not update the locale binding when its text changes, so Window:SetLocale() can restore stale text. Clearing the text also leaves the split rule in place, creating a gap instead of returning to the plain divider layout.
I did not repeat the Label and Paragraph findings here because those changes belong to #90, and their locale issue is already under review there.
Once the Divider setter keeps its locale binding current and restores a continuous rule when its label is cleared, I do not see any other blocking issue in the Divider change.
|
Both fixed. The locale binding goes through Clearing the label also closes the rule back up. The halves sit either side of it with the layout's gap between them, so hiding the label on its own left that gap in the middle of the line. Dropping the far half leaves the near one filling the width, since the layout skips what it cannot see, and that is the plain rule again. Spec covers clearing and setting it back.
|
MaxTCodes
left a comment
There was a problem hiding this comment.
Please remove the Label and Paragraph changes from this PR. Those components already belong to #90 and should remain scoped to that PR.
#92 should include only the Divider implementation, its public API and types, and the tests required for Divider. Keeping these changes separate will make both PRs easier to review and prevent the same code from appearing in two feature PRs.
Please rebase or update this branch so the #92 diff contains only the Divider work.
A Section groups by naming what follows it, so breaking a page into parts meant giving every part a heading whether it wanted one or not. - a faint rule across the page, kept light enough that it does not read as a border against the element strokes either side of it - an optional word in the middle, which splits the rule into halves that share whatever the word leaves, so it stays centred at any length - `line = false` draws no rule and leaves only the room, which is the other thing a divider gets reached for and had no other way of being had - `spacing` sets the room above and below
Same as Label and Paragraph: writing the label directly is undone by the next SetLocale, which re-resolves every localised property from the source it was bound to.
The halves are laid out either side of the label with a gap between them, so hiding the label alone left that gap sitting in the middle of the line. The layout skips what it cannot see, so dropping the far half leaves the near one filling the width, which is the plain rule again.
|
done, rebased onto dev with only the divider commits. the branch was cut off the label/paragraph one which is why it carried them. diff is divider.luau, its CreateDivider on Tab, the two types, and the divider specs. the locale spec here only covers the divider now, the label and paragraph half of it stays on #90. also exported DividerProps and Divider from init while i was rebasing, same as the other elements have. |
MaxTCodes
left a comment
There was a problem hiding this comment.
The Divider implementation looks good now. The earlier locale and clear-label issues are fixed. The Label and Paragraph changes have been removed from this PR, and CI passes on the rebased head.
One item still needs to be addressed before merge. This adds the public Tab:CreateDivider API. The PR notes that it needs public documentation, but no companion docs issue or PR is linked. The feature PR template requires companion work in SiriusSoftwareLtd/docs when a feature affects public documentation.
Please open and link the companion documentation work.
The validation numbers in the PR body are also stale after the rebase. CI currently passes 232 tests with 82.88% line coverage.
We also need to wait for @jensonhirst to sign off on this PR before it can be merged.
I do not see any other blocking issues in the code.
|
companion docs are open as SiriusSoftwareLtd/docs#39, linked in the body now, a Divider page alongside the other elements. body numbers are corrected too, 232 tests and 82.88% after the rebase. |
|
studio validation done, checkbox ticked and the result is in the body. plain rule, rules with a word at default, tight and loose spacing, and room with no rule, all sitting right between the elements either side. recording of the studio run is on #86: https://github.com/user-attachments/assets/8718b07e-4a4c-4606-bf62-e9c7a336b5db |
|
function Divider:Set(text)
if not self.title then
return
endA public method that silently no-ops is a trap. Someone calls it, nothing happens, and there's nothing on screen or in the console saying why. Either build the label lazily on first The constructor takes
Sequencing: this wants to go before #90, they both touch |
Summary
A page can only be broken into parts by naming them, since a Section is a heading. This adds a rule that breaks without saying anything, takes a word in the middle when there is one worth saying, and with no rule at all is simply room.
Related issue
Closes #91
Change type
Changes
src/components/divider.luau: a faint rule across the page, kept light enough that it does not read as a border against the element strokes either side of it.textputs a word in the middle. The rule becomes two halves sharing whatever the label leaves, throughUIFlexItem, so it stays centred at any text length.Setswaps it; a divider built without one has no label to put it in.line = falsedraws no rule and leaves only the room. There is no spacer element, and this is the other thing a divider gets reached for.spacingsets the room above and below, defaulting to 12.Tab:_registerlike every other element, so it reorders with theMove*methods and fades with the page.Compatibility and breaking changes
Tab. Nothing existing changed; no persisted data, themes or public behaviour affected. Not stateful, so no flag is registered.Validation
make ci, including the coverage threshold, or explained below why it does not apply.Test scenarios and results
make ci: 232 tests pass, coverage 82.88% against the 82.88% baseline in this branch.Seton both, the heights for default, custom and line-free spacing, and fading with the page including the line-free case having nothing to fade.Documentation coordination
Tab:CreateDivider.src/types.luaugainsDividerPropsandDivider, and the factory onTab.UI evidence
A plain rule between two groups, one with a word in the middle, one with the line off leaving only the room, tighter and looser spacings, and the label changed through
Set.Checklist
dev.SiriusSoftwareLtd/docs.roblox.yml,sourcemap.json,globalTypes.d.luau, orbuild/.