wagtail / wagtail.org

Wagtail’s official marketing website
https://wagtail.org/
66 stars 60 forks source link

Add Showcase page #446

Closed William-Blackie closed 5 months ago

William-Blackie commented 8 months ago

Hey all this PR creates the latest page type; a Showcase page.

Figma:

What I've done:

Screenshots:

Mobile (light) ![mobile-light](https://github.com/wagtail/wagtail.org/assets/22497837/0f506292-49ec-4649-b800-e0d1148d9e3e)
Mobile (dark) ![mobile-dark](https://github.com/wagtail/wagtail.org/assets/22497837/a6ac0702-bcfd-48c1-8c16-fd47854fbfa7)
Tablet (light) ![tablet-light](https://github.com/wagtail/wagtail.org/assets/22497837/be236758-a69a-41c3-b446-8a492ed61638)
Tablet (dark) ![tablet-dark](https://github.com/wagtail/wagtail.org/assets/22497837/b0cb14f6-ab93-44f8-aca8-0b0db7bdab98)
Desktop (light) ![desktop-light](https://github.com/wagtail/wagtail.org/assets/22497837/0723a8fe-14c3-41b9-85da-0155b8b6f095)
Desktop (dark) ![desktop-dark](https://github.com/wagtail/wagtail.org/assets/22497837/ce2c3850-1ddb-4460-9c4b-6df445342fb0)ry>
William-Blackie commented 7 months ago

Update: On staging.

vossisboss commented 7 months ago

Am reviewing the content and such. Still reviewing but wanted to flag these system warnings that appeared when I ran the branch locally.

?: (templates.E003) 'wagtailcore_tags' is used for multiple template tag modules: 'wagtail.templatetags.wagtailcore_tags', 'wagtailio.project_styleguide.templatetags.wagtailcore_tags'
?: (templates.E003) 'wagtailembeds_tags' is used for multiple template tag modules: 'wagtail.embeds.templatetags.wagtailembeds_tags', 'wagtailio.project_styleguide.templatetags.wagtailembeds_tags'
?: (templates.E003) 'wagtailimages_tags' is used for multiple template tag modules: 'wagtail.images.templatetags.wagtailimages_tags', 'wagtailio.project_styleguide.templatetags.wagtailimages_tags'
?: (templates.E003) 'wagtailsearchpromotions_tags' is used for multiple template tag modules: 'wagtail.contrib.search_promotions.templatetags.wagtailsearchpromotions_tags', 'wagtailio.project_styleguide.templatetags.wagtailsearchpromotions_tags'
vossisboss commented 7 months ago

@William-Blackie Can you confirm this branch is on staging? I'm not seeing the Showcase page child page option on staging but it is on my local copy of the branch. I'll proceed with reviewing the content and such based on my local copy.

zerolab commented 7 months ago

@vossisboss those warnings relate to django-pattern-library and Django 4.2+

vossisboss commented 7 months ago

@William-Blackie Found a few things we should address before pushing to production. Note that I ran my tests locally and not in staging.

The logo display needs some adjustment.

In Ben's design, there is a solid background behind the logos to avoid the issue of the logo clashing with the image. It looks like there is a background there, but the transparency might be off? Either way, it needs to be addressed. Screenshot 2024-02-09 at 4 50 07 PM

Move the pagination below the CTA

When there is more than one page, it would be ideal if the pagination was below the CTA so that it doesn't get shoved down to the bottom of the page when there are multiple pages. Screenshot 2024-02-09 at 5 10 31 PM

Adjust text padding in the mobile display

The text padding in the mobile version seems a bit sparse. It would be nice if there was a bit more space between the words and the edge of the screen. Screenshot 2024-02-09 at 5 09 57 PM

I'm also not sure why the meta text is at the bottom of the page. It would be more useful to have it beneath the intro text. But to be fair to you, it is in Ben's design. So I'll run that one by Ian and Ben.

Thanks so much for putting this together. This will be a real useful page type once we have things spruced up!

benenright commented 7 months ago

Yes, the logos should have a solid white bg and be a fixed size with the logo placed centrally with like 10px padding around it:

image

There should also be a very subtle drop shadow on the logo bg:

image

Sorry for not supplying a mobile design but yes the left/right/bottom padding on the text part of cards is too tight. Make it all 25px.


This:

image

Was supposed to be this footnote:

image

...but with the paging it feels wierd (do we need it?)


Why is the 'next' button so wide in the pagination? Should be more like this:

image

Cheers Ben

vossisboss commented 7 months ago

@benenright FYI, I think the Next button is large in the example I shared because there is no "Previous" button on the first page.

@William-Blackie Ben and I discussed the meta text and where it should be placed. It would be great to have the option to use it closer to the top, right below the line in the prototype that says "Sites and apps made with Wagtail." We would like to have the option to add a bit of descriptive text if needed.

William-Blackie commented 6 months ago

@benenright FYI, I think the Next button is large in the example I shared because there is no "Previous" button on the first page.

@William-Blackie Ben and I discussed the meta text and where it should be placed. It would be great to have the option to use it closer to the top, right below the line in the prototype that says "Sites and apps made with Wagtail." We would like to have the option to add a bit of descriptive text if needed.

Hi @benenright and @vossisboss,

Thanks for the feedback! I'll split my responses a little for readability, anything I don't reference I'll make the changes shortly.

For the logo, I assumed that the images themselves would have backgrounds for two reasons; the first being having a solid background colour could be an accessibility issue, i.e. contrast but I guess that should be a minor worry as most are designed against both light and dark for this reason. The second being to get a consistent sizing it's much easier to supply an image that's to be resized with a background to keep an aspect ratio than allow editors to supply this. But, we can get around this by not scaling the images up, and instead rely on editors to do this. I guess this is mostly force of habit from client work!

I see your points about the pagination, the request came from Ian B as a must, so I that would need to be queried by him. It wasn't in the designs so I repurposed the existing pagination. I'll spend some more time on styling if this is still needed?

For the meta text, "List curated by the Wagtail core team", your request is a little confusing. Are you instead asking for a richtext field beneath the introduction? As this is already a richtext field we could already have this functionality unless you have a different idea in mind.

Let me know what you think; thanks - Will.

vossisboss commented 6 months ago

Hello All. We had a conversation about this project on Slack that didn't make it into the GitHub record. Here are the relevant bits of that conversation for anyone who picks this up:

Screenshot 2024-03-05 at 12 06 02 PM

vossisboss commented 6 months ago

@Tijani-Dia I went ahead and made the field fixes that we needed. I also removed the unnecessary meta field and moved the CTA so that it is above the pagination when there is more than one page displayed.

When I was testing out the pagination by adding more than six items though, I discovered a potential bug. When you click "Next" to get to Page 2 and then "Previous" to get back to Page 1, everything looks fine. But if you repeat going back and forth navigating the pages with the "Next" and "Previous" buttons, something weird happens and the page starts to look like this:

Screenshot 2024-03-11 at 9 37 25 PM

Screenshot 2024-03-11 at 9 37 35 PM

It's like the page is repeating itself somehow. If you could add more than six showcase items to the page and see if you can get it to break in the same way, it would be nice to know if this is really a bug.

We can do a Slack huddle tomorrow if you need me to show you what I mean.

vossisboss commented 6 months ago

@Tijani-Dia and I did some further investigation of this bug and discovered that it is specific to the Firefox browser. We couldn't replicate it in other browsers. Tidiane suspects that is related to the HTMX rendering. Since this bug is relatively hard for a user to trigger, we're going to make a note of it and proceed to testing this branch in staging.

vossisboss commented 6 months ago

@thibaudcolas confirmed the bug in Firefox and found an additional bug in Chrome related to HTMX. Per @thibaudcolas's recommendation, I'm going to remove the HTMX since the vanilla HTML seems to work just fine.

vossisboss commented 6 months ago

@zerolab and/or @thibaudcolas This branch is ready for review if either of you are available to provide a second pair of eyes on it. Please note that the HTMX was removed due to the bug I reported above. A test showcase page is available on the staging site as well if you would like to look at it. I also haven't rebased the branch yet. I'd like to know if any other big changes are needed before I do that.