Move AudioLayout to grid#16133
Conversation
d8a8ce0 to
7ab3d37
Compare
bb701eb to
33da9a8
Compare
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
44275d5 to
93243d7
Compare
| ArticleDisplay[format.display] | ||
| }Layout`} | ||
| /> | ||
| {audioData ? ( |
There was a problem hiding this comment.
Alternatively this fork could (should?) happen inside <MainMedia /> as the audio player here is just a different type of main media. As far as I can tell it's not used anywhere else.
There was a problem hiding this comment.
I think that will ultimately mean the AudioBlockElement gets handled in renderElement along with most other kinds of element? Perhaps then we won't need the separate audio-data.ts file?
One caveat is that renderElement will also get used for body elements. I don't know if @oliverabrahams is aware of a reason we wanted to avoid this when implementing audio articles?
0bc3a39 to
76e769a
Compare
961b98a to
546b54a
Compare
| ArticleDisplay[format.display] | ||
| }Layout`} | ||
| /> | ||
| {audioData ? ( |
There was a problem hiding this comment.
I think that will ultimately mean the AudioBlockElement gets handled in renderElement along with most other kinds of element? Perhaps then we won't need the separate audio-data.ts file?
One caveat is that renderElement will also get used for body elements. I don't know if @oliverabrahams is aware of a reason we wanted to avoid this when implementing audio articles?
| import { AudioPlayerWrapper } from './AudioPlayerWrapper.island'; | ||
| import { formatAudioDuration } from './ListenToArticle.island'; | ||
|
|
||
| export const AudioPlayer = ({ |
There was a problem hiding this comment.
If I've understood this correctly, the component hierarchy now looks like this?
AudioPlayer.tsx
AppsAudioPlayer.island.tsx (the apps version)
AudioPlayerWrapper.island.tsx (the web version)
AudioPlayer/AudioPlayer.tsx
Could this be considered a little confusing, and can we simplify it? I'm happy to discuss approaches if helpful.
There was a problem hiding this comment.
Yeah fair shout. I've changed the top level one so it now essentially flows like this:
DecideAudioPlayer.tsx
AppsAudioPlayer.island.tsx (the apps version)
AudioPlayerWrapper.island.tsx (the web version)
AudioPlayer/AudioPlayer.tsx
Beyond that I'm very happy to have a go at simplifying though I'd argue that opportunity predates this pull request and probably warrants its own, separate one?
There was a problem hiding this comment.
I think prior to this change the apps/web components could have been refactored a bit, but this PR adds an additional wrapper component into the mix? But yeah, a separate PR sounds reasonable!
There was a problem hiding this comment.
Ah what the hell, I've given it a go in 78810ec. With those changes the flow becomes:
AudioPlayer.tsx
AudioPlayerApps.island.tsx (the apps version)
AudioPlayerWeb.island.tsx (the web version)
Much tidier!
Happy to to move out into a separate PR if this one's starting to feel a bit involved but that's the crux of the change.
There was a problem hiding this comment.
I've spun up a standalone PR with those changes to keep things tidy: #16339
a1a3536 to
b63fee8
Compare
60f41d3 to
1add331
Compare
|
@copilot please rebase this PR and resolve conflicts |
I rebased and resolved the conflicts locally at |
78810ec to
a2c6ceb
Compare
Following on from #15428 and #16119, this absorbs AudioLayout into the grid system. I've extracted the audio player into its own component to keep things tidy.
Open question: I started implementing it but thought it was worth asking the question. Why is the body wider for video articles than for audio ones? This only occurs at the
widebreakpoint and to my mind seems needless. If we align their appearance both can be quite happily accommodated by themediagrid layout.Why?
Less code, fewer headaches, reduced existential dread.
Screenshots