Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Row / Col components requiring key #518

Open
LucasFlaquer opened this issue Jul 26, 2023 · 3 comments
Open

Row / Col components requiring key #518

LucasFlaquer opened this issue Jul 26, 2023 · 3 comments
Labels
bug Something isn't working question Further information is requested

Comments

@LucasFlaquer
Copy link
Contributor

Describe the bug

When using the grid components <Row /> and <Col /> always trhows a warning on the console. Aparently the way it's rendered is using a map, would it be possible to fix it turning those components simple divs with the css applied?
image

A clear and concise description of what the bug is.

Steps to reproduce the behavior

  1. create a component with one add two columns,
  2. Open DevTools
  3. It' will trigger the error on the console

Expected vs actual behavior

expected to render cols that are not on a loop without the need of a key prop

@LucasFlaquer LucasFlaquer added the bug Something isn't working label Jul 26, 2023
@rapahaeru rapahaeru added the issue:analyzing Someone is analysing this issue label Jul 31, 2023
@MarcosViniciusPC
Copy link
Contributor

MarcosViniciusPC commented Aug 9, 2023

I didn't quite understand the proposed solution, but I will present the current scenario and the possible solution to the problem.

Current scenario
This warning is shown because the Row component clones all children and displays them using .map, regardless of whether the children are a Col or a div component. The clone of the children aims to pass the ''no-gutters'' prop of the Row component to the children that are Col components, since only the Col component benefits from the 'no-gutters' prop. The Col component uses the prop "no-gutters" to apply spacing in browsers that don't support display:grid.

Alternative to solve the problem
To remove the need for the 'key' prop on the children of Row and preserve the behavior of the Col component, we need to render the children of Row directly (without making a clone and using a .map) and override the styling of the Col children from the Row component, as shown below.

import { Col } from '@quantum/Grid'; const RowStyled = styled.div´ ... ${Col} { ... } ´; ... const Row = (children) => <RowStyled>{children}</RowStyled>);

However, this approach will enlarge the Row component's .css as it will bring the Col component's style to it, increase the Row component's size because of the Col component import, and finally, will not allow the 'no' prop. -gutters' of the Col children takes precedence over the 'no-gutters' prop of the parent component, i.e. the Row component. So I don't see how beneficial it is to fix this "bug" at this time.

Is the proposed solution in any way related to the alternative mentioned above? Can you give me more details of your possible solution?

@MarcosViniciusPC MarcosViniciusPC added question Further information is requested and removed issue:analyzing Someone is analysing this issue labels Aug 9, 2023
@LucasFlaquer
Copy link
Contributor Author

The solution I have propoused have been made without the knologe of te structure of the component. Taking a look inside the code of the <Row /> component I've noticed that it's using a legacy structure (class component) with the cloneElement method. I undestand that you nedd to apply the no-gutthers in the chidlren elements, so, the best solution to this problem here is a refactoring of this legacy structures.

  • Turn the class component into a functional component
  • use this guide to update the cloneElement

this could be a big refactoring, however will resolve all the warnings that came from the component and will update the library

@MarcosViniciusPC
Copy link
Contributor

I will put a task in the backlog to investigate the proposed solution, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants