-
Notifications
You must be signed in to change notification settings - Fork 251
[Remove Vuetify from Studio] Channel not found error page #5480
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
base: unstable
Are you sure you want to change the base?
[Remove Vuetify from Studio] Channel not found error page #5480
Conversation
|
Thanks @vtushar06, we will assign a reviewer next week. |
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.
code changes looks good to me . I also tested manually and it works has stated. Thanks @vtushar06
MisRob
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.
@vtushar06 Even though you say that "Confirmed button interaction works as before", the button is missing and this can be seen on your own screenshot. Would you explain the mismatch between the claim and actual implementation? And just in case the PR description is generated - be careful about this next time and revisit our guidelines - I certainly don't want to nitpick on every word, however this is lately a pattern in wider open-source community and to have trust in continued and healthy collaboration, it is really important for me to see that we can rely on what authors say. Also needs to be fixed - see the original behavior when the button is there.
|
After this is fixed, wanted to note that the button will have blue primary color since that's how the component we're now using does it - that'll be fine and is desired for consistency with other pages. As per note in the issue
|
|
Hello @MisRob ,I sincerely apologize for the confusion earlier. Here’s what’s actually happening: Because of this inconsistency, the console error prevented the button from rendering — which explains why it wasn’t visible in my earlier screenshot and also . I should have caught that during testing before submitting the PR. |
|
Hi @vtushar06, no need to apologize for the missing button itself, that can happen to anyone - that's why we all go through review rounds and later have QA team as well. My main concern was around the PR description itself. As for the button, thanks for debugging - I think it'd be meaningful to fix in this pull request since it's directly dependent. The technical solution you suggest makes sense to me, so feel free to submit - I will let @AllanOXDi coordinate with you further. Thank you. |

Summary
Removed Vuetify dependencies from the Channel not found error page by replacing ChannelDeletedError.vue (which used Vuetify's VBtn and AppError) with the existing ChannelNotFoundError.vue component that uses Kolibri Design System.
Changes made:
Manual verification:
Screenshots:

…
References
Closes #5473
Parent issue: #5060
…
Reviewer guidance
…