feat(grid): add container component #128

frankie303 · 2023-01-11T06:35:40Z

Pull Request

Description

JIRA ticket: https://heycar.atlassian.net/browse/HEYUI-220

With this PR, we introduced polymorphic Container component to our Grid package. Besides that:

by using generics, added TS support to Grid's Row, Col and Container components 🔮 (BREAKING CHANGE). You can see how it works now and how was that before, in the videos below:

Before	After
Screen.Recording.2023-01-12.at.13.10.38.mov	Screen.Recording.2023-01-12.at.08.55.31.mov

added tests

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Documentation change

How do I test this

Check grid demo on different screen sizes
Grid spacings and margins should correspond to this design https://www.figma.com/file/rrvkvQEoVTOHa7MzyKSaoz/OTP-Global-Design-System?node-id=6274%3A139198&t=lsk6VlGUXWRDiqjz-0

Checklist

Did you remember to take care of the following?

npm i – for new NPM dependencies.
npm run lint - to check for linting issues
npm run test - to run unit tests
npm run test:sh:docker - to run screenshot tests, detail instruction

New Feature / Bug Fix

Run unit tests to ensure all existing tests are still passing.
Add new passing unit tests to cover the code introduced by your pr.

Thanks for contributing!

github-actions · 2023-01-11T06:43:42Z

Compiled a new version demo.

coveralls · 2023-01-11T06:45:55Z

Pull Request Test Coverage Report for Build 3912632805

16 of 16 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 97.797%

Totals
Change from base Build 3911817030:	0.04%
Covered Lines:	918
Relevant Lines:	921

💛 - Coveralls

github-actions · 2023-01-11T06:50:55Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟢	Statements	99.5%	1002/1007
🟢	Branches	89.72%	192/214
🟢	Functions	98.36%	60/61
🟢	Lines	99.67%	918/921

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / Container.tsx	100%	100%	100%	100%

Test suite run success

250 tests passing in 37 suites.

Report generated by 🧪jest coverage report action from 3d85b4a

github-actions · 2023-01-11T11:24:44Z

Compiled a new version demo.

github-actions · 2023-01-11T13:44:21Z

Compiled a new version demo.

github-actions · 2023-01-11T14:11:46Z

Compiled a new version demo.

…emove dataTestId

…rops

…eat/HEYUI-220-container-component

github-actions · 2023-01-13T09:20:14Z

Compiled a new version demo.

github-actions · 2023-01-13T09:46:36Z

Compiled a new version demo.

…eat/HEYUI-220-container-component

frankie303 · 2023-01-13T15:36:34Z

packages/grid/src/components/Col.types.ts

-  Component?: keyof JSX.IntrinsicElements;
+  component?: E;


TODO: I think we should adjust other polymorphic components as well in a separate PR with this logic/change

Yes, Pagination will also use something similar

github-actions · 2023-01-13T15:36:51Z

Compiled a new version demo.

lucasmaffazioli

Some small notes:

lucasmaffazioli · 2023-01-16T18:06:13Z

packages/grid/src/components/Col.types.ts

-  Component?: keyof JSX.IntrinsicElements;
+  component?: E;


Yes, Pagination will also use something similar

lucasmaffazioli · 2023-01-16T18:16:30Z

packages/grid/src/components/Container.tsx

+
+import styles from './Container.module.css';
+
+function Container<E extends React.ElementType = 'div'>({


Maybe C could represent container (also in Container.types.ts)?.

ElementType = 'div' could possibly be moved to the types file?

Another possible way to simplify a bit and transfer the typing logic to just one place, something like this could be done:

export interface PropsBasedOnComponent<T> { <C extends React.ElementType = 'div'>( props: { /** * The component used for the root node. * Either a string to use a HTML element or a component. */ component?: C; } & T & Omit<React.ComponentPropsWithoutRef<C>, keyof T>, ): JSX.Element | null; } const Container: PropsBasedOnComponent<ContainerProps> = ({ component, ...rest }) => {

lucasmaffazioli · 2023-01-16T18:27:20Z

packages/grid/src/components/Container.types.ts

@@ -0,0 +1,13 @@
+import React from 'react';
+
+type ContainerBaseProps<E extends React.ElementType> = {


Very very nice typings \o/

lucasmaffazioli · 2023-01-16T18:30:10Z

packages/grid/src/components/Row.tsx

@@ -8,8 +8,8 @@ import { RowProps } from './Row.types';
 import guttersStyles from '../styles/gutters.module.css';
 import styles from './Row.module.css';

-function Row({
-  Component = 'div',
+function Row<E extends React.ElementType = 'div'>({


Same thing applies here, but like you've said on the PR we could have a proper PR to then unify this type of logic

lucasmaffazioli · 2023-01-16T18:34:37Z

Another thing, is it expected for the snapshots/screenshots to have changed position?

esraltintas · 2023-01-17T10:55:29Z

Another thing, is it expected for the snapshots/screenshots to have changed position?

Hi @lucasmaffazioli Yes, It was effecting the other component, when we put the padding in root, with this pr, we will have padding just for the grid.

lucasmaffazioli · 2023-01-17T13:04:33Z

Here's how we've done in our call:

/**
 * `PropsBasedOnComponent` - This interface inherits props from a designated component C through ref
 */
export interface PropsBasedOnComponent<
  ComponentBaseProps,
  DefaultElement extends React.ElementType,
> {
  <Component extends React.ElementType = DefaultElement>(
    props: {
      /**
       * The component used for the root node.
       * Either a string to use a HTML element or a component.
       */
      component?: Component;
    } & ComponentBaseProps &
      Omit<React.ComponentPropsWithoutRef<Component>, keyof ComponentBaseProps>,
  ): JSX.Element | null;
}




export const PaginationItem: PropsBasedOnComponent<
  PaginationItemProps,
  'a'
> = ({
  page,
  isCurrentPage,
  isDisabled,
  type,
  onClick,
  component = 'a',
  ...rest
}) => {

lucasmaffazioli · 2023-01-17T15:04:31Z

Like we've also talked, we were having problems with Storybook showing our props, so I'll approve as it is. 🚀

# [4.0.0](v3.3.1...v4.0.0) (2023-01-17) ### Features * **grid:** add container component ([#128](#128)) ([9eb07f3](9eb07f3)) ### BREAKING CHANGES * **grid:** `Component` prop is renamed as `component` for grid Row, Column and Container * docs(grid): add default value in docs * feat: adjust row widths * revert(grid): remove nested grid styles * test: update screenshot tests * chore: run npm install Co-authored-by: mwagdi <[email protected]>

supcar · 2023-01-17T15:16:07Z

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

fix(grid): remove container styles from grid column

41158f2

frankie303 assigned frankie303 and mwagdi Jan 11, 2023

feat(grid): draft changes for the grid container

a8ff6ce

mwagdi added 2 commits January 11, 2023 14:09

docs(container): adds container to grid on storybook

979d246

test(grid): add test for container component

384a387

test: update screenshots

91762e3

mwagdi marked this pull request as ready for review January 11, 2023 14:22

auto-assign bot requested review from emrahsemizheycar, esraltintas, hcafaq, heyabinanto and pappjanos January 11, 2023 14:22

frankie303 marked this pull request as draft January 11, 2023 15:05

frankie303 removed request for esraltintas, pappjanos, hcafaq, emrahsemizheycar and heyabinanto January 11, 2023 15:15

frankie303 added the work in progress This is a work in progress label Jan 11, 2023

frankie303 changed the title ~~feat(container): add container component~~ feat(grid): add container component Jan 11, 2023

frankie303 added 3 commits January 11, 2023 17:59

refactor(grid): make Container consistent with other components and r…

26c533b

…emove dataTestId

refactor(grid): use consistent types and comments for the Component p…

b28e13a

…rops

refactor(grid): use const assertion for Grid

6569e1c

Merge branch 'main' of https://github.com/hey-car/heycar-uikit into f…

df3fd9c

…eat/HEYUI-220-container-component

test: update screenshot tests

898bfcd

Merge branch 'main' of https://github.com/hey-car/heycar-uikit into f…

ae25439

…eat/HEYUI-220-container-component

frankie303 marked this pull request as ready for review January 13, 2023 15:22

auto-assign bot requested review from emrahsemizheycar, esraltintas, hcafaq, heyabinanto and pappjanos January 13, 2023 15:22

frankie303 removed the work in progress This is a work in progress label Jan 13, 2023

frankie303 requested review from emrahsemizheycar and removed request for emrahsemizheycar January 13, 2023 15:22

chore: run npm install

3d85b4a

frankie303 commented Jan 13, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

lucasmaffazioli reviewed Jan 16, 2023

View reviewed changes

esraltintas approved these changes Jan 17, 2023

View reviewed changes

lucasmaffazioli approved these changes Jan 17, 2023

View reviewed changes

esraltintas merged commit 9eb07f3 into main Jan 17, 2023

esraltintas deleted the feat/HEYUI-220-container-component branch January 17, 2023 15:05

supcar added the released label Jan 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(grid): add container component #128

feat(grid): add container component #128

frankie303 commented Jan 11, 2023 •

edited

Loading

github-actions bot commented Jan 11, 2023

coveralls commented Jan 11, 2023 •

edited

Loading

github-actions bot commented Jan 11, 2023 •

edited

Loading

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 13, 2023

github-actions bot commented Jan 13, 2023

frankie303 Jan 13, 2023 •

edited

Loading

lucasmaffazioli Jan 16, 2023

github-actions bot commented Jan 13, 2023

This comment was marked as resolved.

lucasmaffazioli left a comment •

edited

Loading

lucasmaffazioli Jan 16, 2023

lucasmaffazioli Jan 16, 2023

lucasmaffazioli Jan 16, 2023

lucasmaffazioli Jan 16, 2023

lucasmaffazioli commented Jan 16, 2023

esraltintas commented Jan 17, 2023

lucasmaffazioli commented Jan 17, 2023

lucasmaffazioli commented Jan 17, 2023

supcar commented Jan 17, 2023


		import styles from './Container.module.css';

		function Container<E extends React.ElementType = 'div'>({

		@@ -0,0 +1,13 @@
		import React from 'react';

		type ContainerBaseProps<E extends React.ElementType> = {

feat(grid): add container component #128

feat(grid): add container component #128

Conversation

frankie303 commented Jan 11, 2023 • edited Loading

Pull Request

Description

Type of change

How do I test this

Checklist

Did you remember to take care of the following?

New Feature / Bug Fix

github-actions bot commented Jan 11, 2023

coveralls commented Jan 11, 2023 • edited Loading

Pull Request Test Coverage Report for Build 3912632805

💛 - Coveralls

github-actions bot commented Jan 11, 2023 • edited Loading

Coverage report

Test suite run success

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 13, 2023

github-actions bot commented Jan 13, 2023

frankie303 Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

lucasmaffazioli Jan 16, 2023

Choose a reason for hiding this comment

github-actions bot commented Jan 13, 2023

This comment was marked as resolved.

lucasmaffazioli left a comment • edited Loading

Choose a reason for hiding this comment

lucasmaffazioli Jan 16, 2023

Choose a reason for hiding this comment

lucasmaffazioli Jan 16, 2023

Choose a reason for hiding this comment

lucasmaffazioli Jan 16, 2023

Choose a reason for hiding this comment

lucasmaffazioli Jan 16, 2023

Choose a reason for hiding this comment

lucasmaffazioli commented Jan 16, 2023

esraltintas commented Jan 17, 2023

lucasmaffazioli commented Jan 17, 2023

lucasmaffazioli commented Jan 17, 2023

supcar commented Jan 17, 2023

frankie303 commented Jan 11, 2023 •

edited

Loading

coveralls commented Jan 11, 2023 •

edited

Loading

github-actions bot commented Jan 11, 2023 •

edited

Loading

frankie303 Jan 13, 2023 •

edited

Loading

lucasmaffazioli left a comment •

edited

Loading