Nick Berens GridList Component Test #10

nickberens360 · 2021-09-19T15:05:46Z

Component Created: /components/gallery/List.vue from #3

Stories: GalleryList.stories.js

Imports:

Imports into component

import _dropRight from "lodash/dropRight"
import _takeRight from "lodash/takeRight"
@font-face RM Neue

Imports into story

import WpImage from "~/components/WpImage"
import { data as API } from "~/stories/mock-api.json"

Notes:

A good chunk of the excess time went into seeing what the WpImage component was doing under the hood and playing around with using the figcaption element in place of items with media 404s. I would never bill for that extra time of exploration. Also on an actual component request I would ask first about using the figcaption in the way I did. I liked the way it turned out so decided to include it in the test.

The event @error-video from WpImage seems to return true if @error-image is triggered even if @loaded-video and @playing is true. Not sure if this is a bug with the WpImage @error-video event, or if it's simply my misunderstanding of those particular events. So ended up having to use different combinations of @error-video, @error-image, @loaded-image and @loaded-video to handle different media errors.

I created a story to show the handling of 404s.

In the story GalleryList > List With Errors you will find the following:

Ace Norton: has 404s on all images and the video which results in the WpImage figcaption being displayed instead of the image or video
Michael McCourt: has a video 404 so the image shows instead
Andreas Roth: no errors
Pedro Pinto: the image for Pedro was broken from the start in mock-api.json but the video is fine

Time Report:

This took me 7 hours to build.

Checklist:

I double checked it looks like the designs
I completed any required mobile breakpoint styling
I completed any required hover state styling
I included a working Storybook file
I included a Story that showed some edge case working correctly (long text, short screen, missing image etc.)
I added notes above about how long it took to build this component
I assigned this PR to someone to review

…ions

…en text is hovered

…h mock-api

nickberens360 · 2021-09-19T19:33:34Z

@drewbaker ready for review.

nickberens360 · 2021-09-27T21:50:08Z

@drewbaker just wanted to follow up on this.

nickberens360 · 2021-11-12T19:34:28Z

✌️

drewbaker · 2021-11-12T19:57:32Z

Nick I'm sorry that I lagged on this! Reopening to review today!

drewbaker

Some general notes:

I see scroll bars on Firefox
The center column of names should be center of the screen. The way to do this is to divide the names into 3 columns, and then use flex between.

drewbaker · 2021-11-12T23:00:08Z

stories/GalleryList.stories.js

+    },
+    components: {
+        GalleryList,
+        WpImage,


This isn't being used, can be removed.

drewbaker · 2021-11-12T23:05:11Z

components/gallery/List.vue

+    font-family: 'RM Neue';
+    src: url('../../assets/fonts/RMNeue-Regular.woff2') format('woff2'),
+    url('../../assets/fonts/RMNeue-Regular.woff') format('woff');
+    font-weight: normal;
+    font-style: normal;


Read this for the correct way we use fonts https://github.com/funkhaus/fuxt/wiki/Fonts

Ok this got pretty gross. storybook was breaking because it couldn't resolve the alias ~ to the font files src: url("~/assets/fonts/RMNeue-Regular.woff2") format("woff2"),. After google search hell (I even stumbled upon this :) I pulled down fuxt from your repo and tried setting up the custom font in that project and everything worked as expected so ended up replacing the programming-partners package.json and the nuxt.config.js with the ones from the fuxt repo and it works. I know this isn't ideal but I was going crazy and just wanted to move on.

drewbaker · 2021-11-12T23:12:28Z

components/gallery/List.vue

+    <div class="gallery-list">
+        <ul class="list list-main">


I suspect you don't need both of this elements. Probably can be done with just the ul.

I needed .galler-list to vertically center the hover image and list of items. Also to set the outer width to 1280px. The .list-main class I needed to handle the list columns layout and set max-width to 1180px. Totally cool if there's a better way to handle this but it's pretty much what I landed on.

drewbaker · 2021-11-12T23:25:11Z

components/gallery/List.vue

+                :alt="image.altText"
+                :caption="caption"


These are done by wp-image already

I removed the alt tag but I wanted to use the caption incase both the video and image had errors then I could display the caption as a fallback. You can view the story with errors here.

drewbaker · 2021-11-12T23:26:04Z

components/gallery/List.vue

+                    'has-loaded-media': this.isImageLoaded || this.isVideoLoaded,
+                    'has-full-media-error': this.isVideoError && this.isImageError && !this.isImageLoaded && !this.isVideoLoaded,


If you were just trying to show off classes, then cool.

But I think wp-image adds error and loading classes to itself already.

WpImage adds the class has-error if there is a video error OR image error. So if the video fails to load but the image fallback does load WpImage still adds the class had-error. I needed to add a class based on whether or not one of the two media types loaded successfully .has-loaded-media. I also needed a way to add a class based on whether or not both the image and video fully errored .has-full-media-error and then show the caption if so.

drewbaker · 2021-11-12T23:27:39Z

components/gallery/List.vue

+        shapeListData(array, offset) {
+            let count = array.length
+            let trimLastCount = Math.floor(count / offset)
+            let rightArray = _takeRight(array, trimLastCount)
+            let leftArray = _dropRight(array, trimLastCount)
+
+            return {
+                left: leftArray,
+                right: rightArray
+            }
+        },


If you are going to do a function like this, then you can actually do this outside of methods. If the method has no need for anything Vue related, then make a utility function instead and call that.

Good call and moved to utils

drewbaker · 2021-11-12T23:28:25Z

components/gallery/List.vue

+        flex: 1 1 37%;
+
+        &:last-child {
+            flex: 1 1 63%;


These look like magic numbers, I'd say generally avoid this approach.

Updated, please see the screencast below which explains this madness.

…g trying to resolve ~ alias in font.css. also use server-middleware file from fuxt

nickberens360 · 2021-11-14T20:00:23Z

Some general notes:

I see scroll bars on Firefox

The center column of names should be center of the screen. The way to do this is to divide the names into 3 columns, and then use flex between.

I updated handling of the column layouts but wanted to share a quick screencast of my initial reasoning.
https://www.awesomescreenshot.com/video/6010161?key=211dc6030e4afd8d04052b7d26a6206f

nickberens360 · 2021-12-09T01:40:26Z

@drewbaker safe to assume this was a fail?

drewbaker · 2021-12-09T01:42:05Z

Sorry nick! no safe to assume we’ve just been under the gun trying to get through end of year. I’ll look at this on Monday!

nickberens360 · 2021-12-09T02:41:34Z

Totally understandable. Especially seeing as how all my PR comments add up to a small novel. Just wanted to follow up.

…

On Wed, Dec 8, 2021, 7:42 PM Drew Baker ***@***.***> wrote: Sorry nick! no safe to assume we’ve just been under the gun trying to get through end of year. I’ll look at this on Monday! — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#10 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJZOXRL5DW4MPKRQNKZR33UQACPPANCNFSM5EKNTIBA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

nickberens360 · 2021-12-23T20:58:25Z

@drewbaker just so we are on the same page, I'm going to keep annoying you until you give me an opportunity :)

drewbaker · 2021-12-29T21:07:22Z

@nickberens360 Thanks man you should! I'll need the help in January! We are back from break January 3rd, so I'll get to all this then.

nickberens360 added 24 commits September 16, 2021 06:54

add initial GalleryList component and story

44d3aa8

add data object and import mock data into story

e8cb625

add component logic and default story

ff89a94

add 404 error story

af3d1eb

add comment

379d4eb

add logic to check for multiple error types

425eb7d

group galler-item css rules

74c513e

group remaining css properties

26a72ba

import WpImage into story and component

9d1cbcf

optimize lodash import

4c012d5

move mock-api-error.json data into story

5182ca7

fix merge conflict after sync

f3f0572

change caption styling to have darker background and white text

97556e2

import font into the component because it doesn't appear to set globally

82a6153

change status class names to be more consistent with fuxthaus convent…

97fc015

…ions

move font-size from item-title into link

2af9596

move hover events to h3 and make display-inline to trigger bg only wh…

b09394a

…en text is hovered

decrease ul/li nesting

7181598

check if caption exists in data if not use item title

5469a33

add comment about importing font

6384735

change caption value in story to null instead of empty string to matc…

bad1633

…h mock-api

remove WpImage import from component

0190563

remove list class from gallery-list

c3a8647

lighten caption background a smidge

a3966a8

nickberens360 closed this Nov 12, 2021

drewbaker reopened this Nov 12, 2021

drewbaker requested changes Nov 12, 2021

View reviewed changes

nickberens360 added 13 commits November 13, 2021 12:17

remove WpImage from storybook file

c755240

remove WpImage import

f515b27

refactor loop structure

d2fdc77

remove unused .item-main class

33eb2cb

use fuxt package.json and nuxt.config.js because storybook was failin…

8962200

…g trying to resolve ~ alias in font.css. also use server-middleware file from fuxt

remove font import from component

86ecdd7

move shapeListData to utils

9cbbf83

remove commented out shapeListData method

5a78120

decrease mobile caption size

18428d1

remove component import from story

fcbd842

remove alt tag which is handled by WpImage

5c6f184

add .env.example back in

573a07e

uncomment mouseout

6231703

nickberens360 marked this pull request as draft November 14, 2021 19:26

nickberens360 marked this pull request as ready for review November 14, 2021 20:00

nickberens360 requested a review from drewbaker November 14, 2021 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nick Berens GridList Component Test #10

Nick Berens GridList Component Test #10

nickberens360 commented Sep 19, 2021 •

edited

Loading

nickberens360 commented Sep 19, 2021

nickberens360 commented Sep 27, 2021

nickberens360 commented Nov 12, 2021

drewbaker commented Nov 12, 2021 •

edited

Loading

drewbaker left a comment

drewbaker Nov 12, 2021

nickberens360 Nov 14, 2021 •

edited

Loading

drewbaker Nov 12, 2021

nickberens360 Nov 14, 2021 •

edited

Loading

drewbaker Nov 12, 2021

nickberens360 Nov 14, 2021

drewbaker Nov 12, 2021

nickberens360 Nov 14, 2021 •

edited

Loading

drewbaker Nov 12, 2021

nickberens360 Nov 14, 2021

drewbaker Nov 12, 2021

nickberens360 Nov 14, 2021

drewbaker Nov 12, 2021

nickberens360 Nov 14, 2021 •

edited

Loading

nickberens360 commented Nov 14, 2021

nickberens360 commented Dec 9, 2021

drewbaker commented Dec 9, 2021

nickberens360 commented Dec 9, 2021 via email

nickberens360 commented Dec 23, 2021

drewbaker commented Dec 29, 2021

		'has-loaded-media': this.isImageLoaded \|\| this.isVideoLoaded,
		'has-full-media-error': this.isVideoError && this.isImageError && !this.isImageLoaded && !this.isVideoLoaded,

Nick Berens GridList Component Test #10

Are you sure you want to change the base?

Nick Berens GridList Component Test #10

Conversation

nickberens360 commented Sep 19, 2021 • edited Loading

nickberens360 commented Sep 19, 2021

nickberens360 commented Sep 27, 2021

nickberens360 commented Nov 12, 2021

drewbaker commented Nov 12, 2021 • edited Loading

drewbaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickberens360 Nov 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickberens360 Nov 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickberens360 Nov 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickberens360 Nov 14, 2021 • edited Loading

Choose a reason for hiding this comment

nickberens360 commented Nov 14, 2021

nickberens360 commented Dec 9, 2021

drewbaker commented Dec 9, 2021

nickberens360 commented Dec 9, 2021 via email

nickberens360 commented Dec 23, 2021

drewbaker commented Dec 29, 2021

nickberens360 commented Sep 19, 2021 •

edited

Loading

drewbaker commented Nov 12, 2021 •

edited

Loading

nickberens360 Nov 14, 2021 •

edited

Loading

nickberens360 Nov 14, 2021 •

edited

Loading

nickberens360 Nov 14, 2021 •

edited

Loading

nickberens360 Nov 14, 2021 •

edited

Loading