feat(PageFeatures): Allow the PageFeatures component to render in a grid #1312

judahtanthony · 2021-01-22T22:11:57Z

Overview

Allow the PageFeatures component to render in a grid.

PR Checklist

Related to designs: https://www.figma.com/file/EDtIzUHehl8njnjPStWtYR/Careers-Page---Refresh---1.19.21?node-id=139%3A736
Related to JIRA ticket: REACH-557
I have run this code to verify it works
This PR includes unit tests for the code change

judahtanthony · 2021-01-22T22:12:40Z

packages/gamut-labs/src/landingPage/Feature.tsx

@@ -38,7 +38,7 @@ const FeatureBlock = styled.div`

 type FeaturedMediaProps = {
  featuresMedia?: 'image' | 'icon' | 'stat' | 'none';
-  imgSrc: string;
+  imgSrc?: string;


Now that we have "stat" and "none" features, we shouldn't require this.

Should this be a union of more specific types, then? typeFeatureMediaProps = FeaturedImageProps | FeaturedIconProps | ...?

judahtanthony · 2021-01-22T22:13:04Z

packages/gamut-labs/src/landingPage/Feature.tsx

@@ -65,6 +65,7 @@ const FeaturedMedia: React.FC<FeaturedMediaProps> = ({
        as="div"
        marginTop={48}
        fontSize={{ xs: 44, lg: 64 }}
+        fontWeight="title"


Correction in font-weight.

judahtanthony · 2021-01-22T22:14:36Z

packages/gamut-labs/src/landingPage/PageFeatures.tsx

-        />
-      ))}
-    </FlexContainer>
+    {renderEach(maxCols, features, (feature) => (


Let me know if you think I over engineered this. The idea is the renderEach and figure out how to render this layout. This keep the messy logic out of here.

codecov · 2021-01-22T22:16:05Z

Codecov Report

Merging #1312 (c6841d3) into main (ad53b03) will increase coverage by 0.05%.
The diff coverage is 92.00%.

@@            Coverage Diff             @@
##             main    #1312      +/-   ##
==========================================
+ Coverage   86.53%   86.58%   +0.05%     
==========================================
  Files         302      302              
  Lines        2020     2035      +15     
  Branches      497      500       +3     
==========================================
+ Hits         1748     1762      +14     
- Misses        260      261       +1     
  Partials       12       12

Impacted Files	Coverage Δ
...ckages/gamut-labs/src/landingPage/PageFeatures.tsx	`92.59% <90.00%> (-7.41%)`	⬇️
packages/gamut-labs/src/landingPage/Feature.tsx	`100.00% <100.00%> (+4.76%)`	⬆️

aionate0812 · 2021-01-22T22:57:59Z

packages/gamut-labs/src/landingPage/PageFeatures.tsx

@@ -29,10 +37,57 @@ export type PageFeaturesProps = BaseProps & {
  featuresMedia?: 'image' | 'icon' | 'stat' | 'none';
 };

+const rowRenderEach = (
+  items: PageFeaturesProps['features'],
+  itemRenderer: (item: any) => ReactNode


Is this a specific case where any can be set as the type?. I believe it's encouraged to use any as little as possible

I have removed the any and replaced with an explicit type.

JoshuaKGoldberg

+1 to Alex's comment on the stricter types :)

JoshuaKGoldberg · 2021-01-22T23:06:27Z

packages/gamut-labs/src/landingPage/Feature.tsx

@@ -38,7 +38,7 @@ const FeatureBlock = styled.div`

 type FeaturedMediaProps = {
  featuresMedia?: 'image' | 'icon' | 'stat' | 'none';
-  imgSrc: string;
+  imgSrc?: string;


Should this be a union of more specific types, then? typeFeatureMediaProps = FeaturedImageProps | FeaturedIconProps | ...?

JoshuaKGoldberg · 2021-01-22T23:07:48Z

packages/gamut-labs/src/landingPage/PageFeatures.tsx

+  const size = {
+    xs: 12,
+    sm: 12 / maxCols,
+  } as ResponsiveProperty<ColumnSizes>;


Generally, you don't want to use as: it can hide small but important differences in types. It's meant to be the "I know it's slightly off, just let me do it" operator. Instead, how about inlining this object into the size=?

Hmm I think we'd have to also specify the result of 12 / maxCols IIRC since it won't infer the literal 12 | 6 | 4 | 3 and only number.

Aha, right! I forgot about that. What an irksome lack of feature, TypeScript really should be be able to deduce that here since maxCols is a union type... ah well.

Do you know if there are plans to implement something like that? Seems like integer arithmetic within a limited set would be fairly low overhead?

I'm skimming through their issue tracker and don't see it. You could always request it 😄

Oh actually, I'm finding some references to the TS team not wanting it. The hard part would probably be around edge cases (as always with adding to the type system). An example that would get brought up:

function takesOnePointFive(value: 1.5) { } const two = 2; const three = 3; takesOnePointFive(three / two);

Should that work? If you're saying yes, of course, it would be confusing for users to see division not work, then how does the type system deal with the various quirks of IEEE 754? Is TypeScript going to have to implement a full math engine in the type system, as in microsoft/TypeScript#26382 <- microsoft/TypeScript#15645

This makes sense, greatt find :D Outside of whole numbers this is pretty gnar.

Sorry, so did we decide to inline it, or is the arithmetic going to require a cast anyhow, so might as well cast the whole thing?

@judahtanthony you can do nothing I think 😄 . Up to you.

judahtanthony · 2021-01-27T19:41:06Z

+1 to Alex's comment on the stricter types :)
@JoshuaKGoldberg I think I have addressed all your concerns.

codecademydev · 2021-01-27T20:12:16Z

📬Published Alpha Packages:

@codecademy/[email protected]
@codecademy/[email protected]

codecademydev · 2021-01-27T20:13:09Z

🚀 Styleguide deploy preview ready!

https://6011c93f1872b482ca38abf6--gamut-preview.netlify.app

Deploy Logs

judahtanthony requested a review from a team as a code owner January 22, 2021 22:11

judahtanthony commented Jan 22, 2021

View reviewed changes

aionate0812 reviewed Jan 22, 2021

View reviewed changes

JoshuaKGoldberg suggested changes Jan 22, 2021

View reviewed changes

judahtanthony changed the title ~~Allow the PageFeatures component to render in a grid~~ feat(PageFeatures): Allow the PageFeatures component to render in a grid Jan 27, 2021

judahtanthony force-pushed the jta-reach-557 branch from bfa8f48 to 3733f91 Compare January 27, 2021 18:58

feat(PageFeatures): Allow the PageFeatures component to render in a grid

33901f0

judahtanthony force-pushed the jta-reach-557 branch from 3733f91 to 33901f0 Compare January 27, 2021 19:29

judahtanthony requested a review from JoshuaKGoldberg January 27, 2021 19:38

JoshuaKGoldberg approved these changes Jan 27, 2021

View reviewed changes

Merge branch 'main' into jta-reach-557

c6841d3

judahtanthony merged commit 4532a1e into main Jan 27, 2021

judahtanthony deleted the jta-reach-557 branch January 27, 2021 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(PageFeatures): Allow the PageFeatures component to render in a grid #1312

feat(PageFeatures): Allow the PageFeatures component to render in a grid #1312

judahtanthony commented Jan 22, 2021

judahtanthony Jan 22, 2021

JoshuaKGoldberg Jan 22, 2021

judahtanthony Jan 22, 2021

judahtanthony Jan 22, 2021

codecov bot commented Jan 22, 2021 •

edited

Loading

aionate0812 Jan 22, 2021

judahtanthony Jan 27, 2021

JoshuaKGoldberg left a comment •

edited

Loading

JoshuaKGoldberg Jan 22, 2021

JoshuaKGoldberg Jan 22, 2021

codecaaron Jan 22, 2021 •

edited

Loading

JoshuaKGoldberg Jan 22, 2021

codecaaron Jan 23, 2021

JoshuaKGoldberg Jan 23, 2021

JoshuaKGoldberg Jan 23, 2021 •

edited

Loading

codecaaron Jan 23, 2021

judahtanthony Jan 27, 2021

JoshuaKGoldberg Jan 27, 2021

judahtanthony commented Jan 27, 2021

codecademydev commented Jan 27, 2021

codecademydev commented Jan 27, 2021

feat(PageFeatures): Allow the PageFeatures component to render in a grid #1312

feat(PageFeatures): Allow the PageFeatures component to render in a grid #1312

Conversation

judahtanthony commented Jan 22, 2021

Overview

PR Checklist

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

codecov bot commented Jan 22, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecaaron Jan 22, 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

JoshuaKGoldberg Jan 23, 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

judahtanthony commented Jan 27, 2021

codecademydev commented Jan 27, 2021

📬Published Alpha Packages:

codecademydev commented Jan 27, 2021

codecov bot commented Jan 22, 2021 •

edited

Loading

JoshuaKGoldberg left a comment •

edited

Loading

codecaaron Jan 22, 2021 •

edited

Loading

JoshuaKGoldberg Jan 23, 2021 •

edited

Loading