-
Notifications
You must be signed in to change notification settings - Fork 262
Refactor publish side panel #5603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor publish side panel #5603
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexVelezLl and I had a quick chat on Slack - no concerns about the bug fix, but @AlexVelezLl is going to do a little bit of code clean up of the pre-existing code before we merge.
9498094 to
63eef8c
Compare
|
Noting that currently the invalid language message is not shown because of this bug in KDS, once fixed in KDS, it will be fixed automatically here. |
63eef8c to
44f14d5
Compare
rtibbles
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests provide good coverage, manual testing checks out, and the code read through makes sense.
Just one comment about the slightly confusing value.value, but not a blocker.
| language.value.value && | ||
| language.value.value !== currentChannel.value?.language | ||
| ) { | ||
| if (newChannelLanguage.value.value !== currentChannel.value.language) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see now that this is coming from the language options for a dropdown - I must admit the "double value" reference here was incredibly confusing for a while. May be worth a code comment to save others the mental working through to understand why this is like this.
44f14d5 to
1e372fe
Compare
|
Thanks @rtibbles! |
Summary
Fixes an incorrect reference to
channelLanguageExistson theshowLanguageDropdowncomputed property. This meant that whenchannelLanguageExists.valuewas false, the language dropdown was not shown, but thepublishbutton still required a value, making the publish modal unusable.Before
Grabacion.de.pantalla.2025-12-11.a.la.s.9.18.59.a.m.mov
After
Grabacion.de.pantalla.2025-12-11.a.la.s.9.20.00.a.m.mov
Reviewer guidance
This
showLanguageDropdownis a migration from the old publish modal. When creating the new publish side panel, this difference was overlooked.