Revert "chore: Keep working with all transitions" #171
Conversation
Summary of ChangesHello @gocanto, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request serves to revert a prior change (PR #170) that implemented a custom transition strategy for several components. The revert removes the custom Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request reverts a previous change related to transitions, switching from a custom cross-fade implementation back to Vue's built-in out-in transition mode. This simplifies the codebase by removing the need for custom CSS to handle overlapping elements. My review focuses on improving accessibility for the components that show loading states. I've noticed that in many places, the containers for dynamically loaded content are not marked as ARIA live regions. Adding role="status" to these containers would significantly improve the experience for screen reader users by announcing when content is loading and when it becomes available. I've made several suggestions to apply this pattern consistently. Additionally, I've pointed out a potential accessibility regression in PostPage.vue where aria-hidden="true" was removed from a skeleton loader. It's best practice to hide purely visual skeletons from assistive technologies to avoid confusion.
| <template> | ||
| <div class="relative min-h-[18rem] fade-transition-wrapper"> | ||
| <transition name="fade" appear> | ||
| <div class="relative min-h-[18rem]"> |
There was a problem hiding this comment.
To improve accessibility, consider adding role="status" to this container. This will make it a live region, announcing to screen reader users when the content (e.g., switching from a skeleton loader to the actual content) has been updated. This pattern is already used in TagPostsPage.vue, and applying it here would improve consistency and user experience.
<div class="relative min-h-[18rem]" role="status">
| <template> | ||
| <div class="relative min-h-[200px] fade-transition-wrapper"> | ||
| <transition name="fade" appear> | ||
| <div class="relative min-h-[200px]"> |
There was a problem hiding this comment.
To improve accessibility, consider adding role="status" to this container. This will make it a live region, announcing to screen reader users when the content (e.g., switching from a skeleton loader to the actual content) has been updated. This pattern is already used in TagPostsPage.vue, and applying it here would improve consistency and user experience.
<div class="relative min-h-[200px]" role="status">
| <h2 class="h2 font-aspekta text-slate-700 dark:text-slate-300">Let's Connect</h2> | ||
| <div class="relative min-h-[5rem] fade-transition-wrapper"> | ||
| <transition name="fade" appear> | ||
| <div class="relative min-h-[5rem]"> |
There was a problem hiding this comment.
To improve accessibility, consider adding role="status" to this container. This will make it a live region, announcing to screen reader users when the content (e.g., switching from a skeleton loader to the actual content) has been updated. This pattern is already used in TagPostsPage.vue, and applying it here would improve consistency and user experience.
<div class="relative min-h-[5rem]" role="status">
| <div class="relative min-h-[25rem] fade-transition-wrapper"> | ||
| <transition name="fade" appear> | ||
| <PostPageSkeletonPartial v-if="isLoading" key="skeleton" aria-hidden="true" /> | ||
| <div class="relative min-h-[25rem]"> |
There was a problem hiding this comment.
To improve accessibility, consider adding role="status" to this container. This will make it a live region, announcing to screen reader users when the content (e.g., switching from a skeleton loader to the actual content) has been updated. This pattern is already used in TagPostsPage.vue, and applying it here would improve consistency and user experience.
<div class="relative min-h-[25rem]" role="status">
| <PostPageSkeletonPartial v-if="isLoading" key="skeleton" aria-hidden="true" /> | ||
| <div class="relative min-h-[25rem]"> | ||
| <transition name="fade" appear mode="out-in"> | ||
| <PostPageSkeletonPartial v-if="isLoading" key="skeleton" /> |
There was a problem hiding this comment.
This revert removes aria-hidden="true" from the skeleton loader. While this is part of the revert, it's a good practice to hide skeleton loaders from screen readers as they are purely visual placeholders and can be confusing. I recommend adding aria-hidden="true" back to this component, especially since other skeleton components in the project seem to be hidden from assistive technologies (e.g., as indicated by tests for ResumePage).
<PostPageSkeletonPartial v-if="isLoading" key="skeleton" aria-hidden="true" />
| <!-- Page content --> | ||
| <div class="text-slate-500 dark:text-slate-400 relative fade-transition-wrapper"> | ||
| <transition name="fade" appear> | ||
| <div class="text-slate-500 dark:text-slate-400 relative"> |
There was a problem hiding this comment.
To improve accessibility, consider adding role="status" to this container. This will make it a live region, announcing to screen reader users when the content (e.g., switching from a skeleton loader to the actual content) has been updated. This pattern is already used in TagPostsPage.vue, and applying it here would improve consistency and user experience.
<div class="text-slate-500 dark:text-slate-400 relative" role="status">
| <h2 class="font-aspekta text-xl font-[650] mb-6">Articles</h2> | ||
| <div class="relative min-h-[20rem] fade-transition-wrapper"> | ||
| <transition name="fade" appear> | ||
| <div class="relative min-h-[20rem]"> |
There was a problem hiding this comment.
It's great that the parent <section> has role="status". To make the live region more specific to the content being loaded, you could move role="status" to this div which directly wraps the <transition>. This would make the announcements more precise.
<div class="relative min-h-[20rem]" role="status">
| <!-- Articles list --> | ||
| <div class="relative min-h-[24rem] fade-transition-wrapper"> | ||
| <transition name="fade" appear> | ||
| <div class="relative min-h-[24rem]"> |
There was a problem hiding this comment.
To improve accessibility, consider adding role="status" to this container. This will make it a live region, announcing to screen reader users when the content (e.g., switching from a skeleton loader to the actual content) has been updated. This pattern is already used in TagPostsPage.vue, and applying it here would improve consistency and user experience.
<div class="relative min-h-[24rem]" role="status">
| <!-- Cards --> | ||
| <div class="relative min-h-[300px] fade-transition-wrapper"> | ||
| <transition name="fade" appear> | ||
| <div class="relative min-h-[300px]"> |
There was a problem hiding this comment.
To improve accessibility, consider adding role="status" to this container. This will make it a live region, announcing to screen reader users when the content (e.g., switching from a skeleton loader to the actual content) has been updated. This pattern is already used in TagPostsPage.vue, and applying it here would improve consistency and user experience.
<div class="relative min-h-[300px]" role="status">
| <!-- Cards --> | ||
| <div class="relative min-h-[300px] fade-transition-wrapper"> | ||
| <transition name="fade" appear> | ||
| <div class="relative min-h-[300px]"> |
There was a problem hiding this comment.
To improve accessibility, consider adding role="status" to this container. This will make it a live region, announcing to screen reader users when the content (e.g., switching from a skeleton loader to the actual content) has been updated. This pattern is already used in TagPostsPage.vue, and applying it here would improve consistency and user experience.
<div class="relative min-h-[300px]" role="status">
Reverts #170