-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/2090 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
Size Change: +8.05 kB (+1%) Total Size: 907 kB
ℹ️ View Unchanged
|
@@ -5,12 +5,14 @@ | |||
v-bind="$attrs" |
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.
Maybe rename this component to VBackLink
?
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.
We can remove this change entirely now, right? The component is no longer used in the new report page.
I designed this quick idea that taps into existing styles and your approach.
DescriptionThe page has the Footer content and the header version where switcher and filters are disabled. This version was discussed in a different ticket but is not fully defined yet. In the result area, the wrapper is All text styles and buttons are existing components. The mockup lists them better. What do you think? If we go with this idea, I will polish minor mockup details for its implementation. Design fileFigma: Full page report |
Thanks, @panchovm! This looks great. I can implement these suggestions pretty quickly once you've made final tweaks. No objections on the overall design. |
Done. The file has been updated. Here are the mockups and a wireframe. Select View > Layout Grids to see the grid settings of all mockups. |
@panchovm and @obulat I think this PR is in a good place to merge. The only change that wasn't made, which could be in a subsequent PR, is to update the type sizes of headings and move the button to the left. I think those changes require a bit more work to add props and more configuration to the What do you think? |
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.
Given the priority this ticket has, the outcome meets the functional requirement. We can improve the styles later.
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.
LGTM, the button needs to be fixed to be a link, rather than a button.
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.
Just leaving a suggested change to fix the link issue. I've tested it out locally and the page works fine aside from this issue.
@@ -5,12 +5,14 @@ | |||
v-bind="$attrs" |
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.
We can remove this change entirely now, right? The component is no longer used in the new report page.
src/pages/image/_id/report.vue
Outdated
variant="secondary-bordered" | ||
:href="`/image/${image.id}`" | ||
size="disabled" | ||
class="p-2 text-sr" |
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.
variant="secondary-bordered" | |
:href="`/image/${image.id}`" | |
size="disabled" | |
class="p-2 text-sr" | |
as="VLink" | |
variant="secondary-bordered" | |
:href="`/image/${image.id}`" | |
size="disabled" | |
class="p-2 text-sr text-black" |
The additional class is required to prevent the button text from switching to the pink used for links (not sure if this is intentional or a bug in the component).
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.
LGTM, strange about the test failures though 😕
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 think rebasing onto main will fix the testing problems.
Co-authored-by: Olga Bulat <obulat@gmail.com>
Playwright Failure Test Results It looks like some of the Playwright tests failed. You can download the Playwright trace https://github.com/WordPress/openverse-frontend/actions/runs/3965536056 Read more about how to use this artifact here: https://github.com/WordPress/openverse-frontend/blob/main/test/playwright/README.md#debugging |
Proof-of-concept to create a reporting view for each image. This would allow Openverse integrations to have a simple reporting URL for each image.
Todo