add toggle to switch between old and new UI#410
Conversation
| import { daFetch } from '../../utils/daFetch.js'; | ||
|
|
||
| const TOGGLE_MAP = { '/edit': '/canvas', '/canvas': '/edit', '/browse': '/', '/': '/browse' }; | ||
| const cache = {}; |
There was a problem hiding this comment.
Does this grow for ever? if a user navigates through many org/site combos, entries accumulate for the session lifetime. not critical, but worth noting and also for me to better understand the da architecture in general.
| }; | ||
|
|
||
| check(); | ||
| window.addEventListener('hashchange', check); |
There was a problem hiding this comment.
never removed? if the nav component disconnects and reconnects, we get duplicate listeners. we need cleanup in disconnectedCallback.
| .then((r) => (r?.ok ? r.json() : null)) | ||
| .then((json) => { | ||
| const sheet = json?.data?.data ?? json?.data; | ||
| return !!sheet?.find(({ key: k, value }) => k === 'nx-toggle' && value === 'true'); |
There was a problem hiding this comment.
I know this may be nit, because I genuinely dislike single letter named vars. I understand that in sheet?.find(({ key: k, value }) => ...). k is used to avoid shadowing the outer key param. fair reason but confusing to read. why not rename the outer const to cacheKey?
| height: 20px; | ||
| } | ||
|
|
||
| .nx-nav-toggle { |
There was a problem hiding this comment.
So many magic numbers....
| } | ||
|
|
||
| .nx-nav-toggle:hover { | ||
| background: #e1e1e1; |
There was a problem hiding this comment.
background: #e1e1e1. no token, no light-dark(). won't this look wrong in the dark mode?
| btn.className = 'nx-nav-toggle'; | ||
| btn.innerHTML = `<svg class="icon"><use href="#S2IconLayout20N-icon"/></svg><span>${label}</span>`; | ||
| btn.addEventListener('click', () => { | ||
| const { pathname: p, hash: h, search } = window.location; |
There was a problem hiding this comment.
the listener closure captures pathname at call time via window.location. that's fine. but if the toggle is created once and the user navigates, the label ("Old UI"/"New UI") goes stale. is the toggle re-created on every hashchange? (looks like yes, via remove + re-add, so probably ok.)
| const label = (pathname === '/canvas' || pathname === '/browse') ? 'Old UI' : 'New UI'; | ||
| const btn = document.createElement('button'); | ||
| btn.className = 'nx-nav-toggle'; | ||
| btn.innerHTML = `<svg class="icon"><use href="#S2IconLayout20N-icon"/></svg><span>${label}</span>`; |
There was a problem hiding this comment.
btn.innerHTML = <svg...>${label}is safe here sincelabel is from a hardcoded ternary. but innerHTML with dynamic content is a pattern to generally avoid..
| } | ||
|
|
||
| class Nav extends HTMLElement { | ||
| constructor() { |
There was a problem hiding this comment.
async connectedCallback() is risky... if loadNav() throws, the rejection is unhandled (no caller awaits the lifecycle method). also, setupToggle now runs only after loadNav finishes, meaning the toggle won't show until nav fully loads. is that intentional? why not fire-and-forget setupToggle independently?
2 participants
https://ew-toggle--da-nx--adobe.aem.page
https://da.live/?nx=ew-toggle#/exp-workspace/frescopa - Here the toggle should show to switch to new UI
https://da.live/browse?nx=ew-toggle#/exp-workspace/frescopa - Here the toggle should show to switch to old UI
https://da.live/edit?nx=ew-toggle#/exp-workspace/frescopa/index - Here the toggle should show to switch to new UI
https://da.live/canvas?nx=ew-toggle#/exp-workspace/frescopa/index - Here the toggle should show to switch to old UI
https://da.live/?nx=ew-toggle#/exp-workspace/foobar - here the toggle shouldn't show
https://da.live/browse?nx=ew-toggle#/exp-workspace/foobar - here neither