-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
# Conflicts: # pnpm-lock.yaml # src/locales/po-files/openverse.pot # src/pages/sources.vue
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.
Wow, thanks! I changed the base branch to main
, otherwise there were 392 files to review 😅 . We can just merge this as a replacement to #1093 I think.
The changes look perfect to me; LGTM!
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.
Tested locally and looks good to me. One question before approving but otherwise looks good.
Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>
@sarayourfriend could you re-review this one before your time off? seems like a quick win. |
<template> | ||
<div class="pt-5 md:pt-10" dir="ltr"> | ||
<div | ||
class="px-6 lg:px-0 mb-10 lg:mb-30 md:max-w-4xl xl:max-w-5xl prose prose-sm md:prose-base mx-auto max-w-none prose-headings:font-bold lg:prose-headings:text-3xl prose-h1:text-4xl prose-h1:text-bold lg:prose-h1:text-6xl md:prose-h1:mb-10 prose-h3:text-2xl prose-h3:font-semibold md:prose-h3:mt-10" |
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.
This massive list of prose-*
classes also scares me. It has all the features of the worst parts of Tailwind IMO (namely, a super long and inscrutable list of class names that are difficult to edit, read, and get a full picture of).
Adding our own prose
class to tailwind.css
with these styles using the @apply
rule would probably be easier to read and maintain 🤷 Just my own feeling about this plugin 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.
The pages LGTM. I don't really like the plugin but I can see that it makes things easier here in some sense (though IMO harder long term) so I won't block on it.
@sarayourfriend the primary reason I'd like to use the plugin is so we have more raw html tags in use and can easily replace things, like |
Fixes
Fixes #1090 by @zackkrida
Description
This PR attempts to finish up the work started in #1093 to get it merged :)
It merges main and applies the following #1093 code review suggestions:
On desktop
On mobile
The following suggestions were not implemented:
Other changes:
scss
removed from the pages stylesVContentPage
name added so that it can be imported in the components.Testing Instructions
Check that the content pages look similar to the Sources page mockup, and are not broken.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin