Conversation
…e standard Dash elements.
There was a problem hiding this comment.
Pull request overview
Updates the Community Winds Dash UI to reflect the ACCAP logo/name update work and to remove usage of the deprecated dash_dangerously_set_inner_html component for newer Dash versions.
Changes:
- Replaced
DangerouslySetInnerHTMLTOC markup with explicit Dashhtml.*components and added a small CSS helper class for TOC anchor sizing. - Replaced one
DangerouslySetInnerHTMLblock withdcc.Markdown(dangerously_allow_html=True)to preserve inline SVG/icon rendering. - Updated Pipenv dependency set/lockfile for newer Dash/Plotly/Flask versions and moved Flask to runtime dependencies.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
gui.py |
Reworks TOC rendering and replaces inline HTML injection usage; adjusts path prefix handling and includes an HTML-enabled Markdown block. |
assets/99_app.css |
Adds .anchor-link-size and reformats CSS; contains TOC heading selector that may no longer match updated markup. |
Pipfile |
Removes dash_dangerously_set_inner_html and moves flask from dev to runtime dependencies. |
Pipfile.lock |
Locks updated dependency versions; still includes dash-dangerously-set-inner-html and introduces potential deployment mismatch if requirements.txt is used. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| path_prefix = os.getenv("DASH_REQUESTS_PATHNAME_PREFIX") | ||
| assert path_prefix, f"DASH_REQUESTS_PATHNAME_PREFIX needs to be set!" | ||
| path_prefix = os.getenv("DASH_REQUESTS_PATHNAME_PREFIX", "/") |
There was a problem hiding this comment.
DASH_REQUESTS_PATHNAME_PREFIX used to be required; defaulting it to "/" can silently break asset URLs when the app is deployed under a sub-path (the README says this env var must be set appropriately on EB). Consider keeping the explicit failure (assert/raise) or at least logging a clear warning when it’s missing so misconfiguration is caught early.
| html.H4( | ||
| "Explore observed winds (1980-2014)", className="title is-6" | ||
| ), | ||
| html.Ol( |
There was a problem hiding this comment.
The TOC section headings are now H4 elements, but the existing TOC CSS targets #toc li h3 (and the previous markup used an h3). This will likely change spacing/inline-block styling in the TOC; either revert these headings to H3 or adjust the CSS selectors accordingly to keep the layout identical.
| #toc li h3 { | ||
| display: inline-block; | ||
| font-size: 1.1rem; | ||
| margin-bottom: 0.5rem; | ||
| margin-top: 0.5rem; | ||
| display: inline-block; | ||
| font-size: 1.1rem; | ||
| margin-bottom: 0.5rem; | ||
| margin-top: 0.5rem; | ||
| } |
There was a problem hiding this comment.
TOC heading styling is scoped to #toc li h3, but the updated TOC markup uses H4 for the section headings. Update this selector (e.g., include h4) or align the markup so the intended TOC heading styles still apply.
| dcc.Markdown( | ||
| dangerously_allow_html=True, | ||
| children='<p class="content is-size-5 camera-icon">Click the <span><svg viewBox="0 0 1000 1000" class="icon" height="1em" width="1em"><path d="m500 450c-83 0-150-67-150-150 0-83 67-150 150-150 83 0 150 67 150 150 0 83-67 150-150 150z m400 150h-120c-16 0-34 13-39 29l-31 93c-6 15-23 28-40 28h-340c-16 0-34-13-39-28l-31-94c-6-15-23-28-40-28h-120c-55 0-100-45-100-100v-450c0-55 45-100 100-100h800c55 0 100 45 100 100v450c0 55-45 100-100 100z m-400-550c-138 0-250 112-250 250 0 138 112 250 250 250 138 0 250-112 250-250 0-138-112-250-250-250z m365 380c-19 0-35 16-35 35 0 19 16 35 35 35 19 0 35-16 35-35 0-19-16-35-35-35z" transform="matrix(1 0 0 -1 0 850)"></path></svg></span> icon in the upper–right of each chart to download it.</p>', | ||
| ), |
There was a problem hiding this comment.
This replacement still enables raw HTML rendering via dangerously_allow_html=True. Even though the current content is static, it reintroduces an HTML-injection surface and doesn’t fully align with the PR description of moving to “standard Dash elements”. Consider building this paragraph with dash.html components (and an SVG asset) to avoid needing any “dangerously” HTML rendering.
This PR updates the ACCAP logo for the Community Winds web application. It also updates the use of DangerouslySetInnerHTML calls since those are no longer available in newer versions of Dash that this was trying to execute on. They are replaced with standard Dash elements that do the same job and should be identical. One new class was added to the CSS for anchor-link-size to make the anchor links match the size in the original application.
Closes #35