docs(dashboards): address the review on the control-URL section - #11691
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
github-actions
Bot
added
the
pr:community
Follow-up to #11687, which I merged on green checks before reading the review that had landed two minutes earlier. All five findings hold; four are mine, one is pre-existing. - The granularity list said `day`…`year`, contradicting *Allowed granularities* on the same page: `second`, `minute` and `hour` are offered for `TIMESTAMP` / `DATETIME` dimensions, so a reader building an hourly `tg_` link would have concluded the parameter couldn't express it. Both pages now name the sub-day grains and point at the switcher's allowed list as the authority. - "Published dashboards only" read as scoping the whole feature, contradicting the embed page's "works on both regular and published dashboards". Both are true — the restriction is on the outbound write, not the inbound read — so the bullet now says which direction it constrains. - The section said the children's values travel but not what the recipient's PARENT dropdown shows. Confirmed against the implementation: a parent's selection is runtime-only state and never serialized, so it falls back to the widget's saved `defaultOptionId`. Documented, because a recipient seeing the children set while the parent reads something else looks like a bug. - The link text still said "Pre-set dashboard filters via URL" after this work renamed that heading to include granularities. - The embed example carries raw `{`, `"` and `}`. Pre-existing, but that section is now the canonical reference for hand-written links, so it says to percent-encode when building the URL programmatically. Also unwrapped the new prose to one paragraph per line, matching the rest of `controls.mdx` (the hard-wrapped block would have made every later diff there noisier).
6283f63 to
3e50e59
Compare
|
Claude finished @mngr's task in 1m 59s —— View job Docs review — changes are accurate and land all five findings; 3 low-severity follow-ups, all on the embed pageFull review
Verified
Not verifiable hereThe behavioral claims behind findings 2 and 3 — parent selection being runtime-only Findings
None of these block the merge — each is a small addition to text this PR already touches. Worth keepingThe finding-2 rewrite is the strongest edit in the set: naming the direction of the restriction rather than the feature turns two apparently conflicting sentences into one coherent rule, and the new bullet reads better than the one it replaces. Finding 3's addition earns its length — "which may not be the option the sharer picked" pre-empts the bug report that behavior would otherwise generate. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Three low-severity follow-ups from the review of the previous commit, all on the embed page and all in text this work already touched: - The "reverse direction works too" paragraph still claimed write-back without the published-only qualifier — the same contradiction the controls page just had fixed on its read side, surviving here on the write side. - The percent-encoding note pointed at "constructing the URL programmatically", but the failure a reader will actually hit is pasting the example into the `src="…"` of the iframe snippet directly above it, where the raw quote closes the attribute and truncates the URL. Says that instead. - "the switcher's allowed granularities" is now a link, as it already is in the parallel sentence on the controls page; a reader here had no way to reach the list.
|
Claude finished @mngr's task in 1m 6s —— View job Re-review of
|
| # | Prior finding | Status |
|---|---|---|
| 1 | Write-back claimed unqualified at dashboards.mdx:91 |
Fixed. Now "The reverse direction works on published dashboards: when a viewer changes a control there…", followed by "Reading the parameters works anywhere; it's the writing that is published-only." That last sentence is a near-verbatim echo of the controls.mdx:211 bullet, so the two pages now state the same rule in the same terms — which is the right call for a contradiction that survived one round already. |
| 2 | Percent-encoding note scoped to "programmatically" | Fixed, and better than my suggestion. "pasted as-is into the src=\"…\" of the iframe snippet above, its raw \" closes the attribute and truncates the URL" names the mechanism and the symptom, so a reader who already hit the silent truncation can match it to this note. |
| 3 | Bare "allowed granularities" on the embed page | Fixed. Link resolves: ### Allowed granularities at controls.mdx:86 auto-anchors to #allowed-granularities. The link text wraps across a line break, which is valid markdown — Mintlify renders it as one link. |
Verified on the current tree
#allowed-granularities,#default-granularity,#parent,#sharing-the-current-selection,#visibility— all resolve to real headings incontrols.mdx.#user-attribute-default(controls.mdx:209) resolves to the#### User attribute defaultheading at line 156 — carried over from docs: a parent control can take its default from a user attribute #11689, still good after this PR's rewrite of the surrounding bullets.[ref-embed-url-filters](controls.mdx:268) →/embedding/iframe/dashboards#pre-set-dashboard-filters-via-url, which matches the explicit{#pre-set-dashboard-filters-via-url}anchor atdashboards.mdx:62. The heading rename remains non-breaking.- Wrapping conventions respected on both files:
controls.mdxunwrapped,dashboards.mdxhard-wrapped ~80. - Docs-only change; nothing under
packages/orrust/touched, so no test or build surface.
One non-blocking observation
dashboards.mdx:87 and :94 now both carry the read/write scoping — "This works on both regular and published (embedded) dashboards" and then "Reading the parameters works anywhere; it's the writing that is published-only." The second is the load-bearing one; the first is now slightly redundant with it. Not worth a commit on its own — the redundancy errs toward clarity on exactly the point that was previously misread, which is the right side to err on.
Worth keeping
The direction-scoping now reads identically on both pages, which is what makes it durable: a future editor touching one page has the other's phrasing to match rather than a paraphrase to drift from. And the parent dropdown opens on whatever default it resolves for them, which may not be the option the sharer picked clause at controls.mdx:210 is the single most useful sentence added across both PRs — it describes observable behavior rather than the serialization mechanism, so it survives implementation changes.
I did not re-verify the behavioral claims (parent selection being runtime-only, write-back being published-only) — as noted last round, the dashboard builder implementation isn't in this monorepo, so the PR author's reading of it remains the only evidence available here.
1 participant
What
Follow-up to #11687, which I merged on green checks two minutes after the review landed — before reading it. All five findings hold, so this fixes them. Four are mine; one is pre-existing.
day…year, contradicting Allowed granularities on the same page —second,minute,hourare offered forTIMESTAMP/DATETIMEdimensions. Both pages now name the sub-day grains and point at the switcher's allowed list as the authority.{,",}— pre-existing, but that section is now the canonical hand-written-URL reference, so it says to percent-encode when building programmatically.Plus the wrap nit: the new prose is unwrapped to one paragraph per line, matching the rest of
controls.mdx.On finding 3
The reviewer flagged its own suggested wording as unverified ("worth confirming against the implementation"). I checked: a parent control's selection is runtime-only
useStateinuseDashboardParents, never serialized, andresolveParentSelectionfalls back to the widget's saveddefaultOptionId. So the recipient does see the children's shared values while the parent dropdown reads the dashboard's own default — which is exactly why it's worth stating, since it otherwise looks like a bug.