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

Data file upload components #3

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

davinov
Copy link

@davinov davinov commented Jun 23, 2022

Here is the start of react components to handle data file uploads: both components should use a common container, with two different themes.
Rest to do:

  • handle state when uploading
  • factor file input logic
  • tests!

Many questions:

  • multiple themes vs multiple components
  • common way to handle stuff that were "out of the box" with vue (class names composition, native events on components, styles colors, etc.)

Comment on lines +28 to +31
const style: CSSProperties = {};
if (onClick) {
style.cursor = 'pointer';
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tu pourrais ici par exemple passer une prop a StyledContainer qui serai isClickable de type boolean et adapter StyledContainer comme ça.

import styled, { css } from 'styled-components';

const StyledContainer = styled.div<{ isClickable: boolean }>`
  ${props => props.isClickable && css`cursor: pointer`}
`

Comment on lines +23 to +26
let classes = "card";
if (className) {
classes += " " + className;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si tu veux composer des classNames (malgré qu'on utilise styled-components), tu peux utiliser la librarie classnames.
Cette library exporte une function (import cx from 'classnames') que tu peux utiliser comme ça:

const classes = cx('card', className, {
  'a-condition-classname': aBoolean
})


interface CardProps {
className?: string;
theme?: 'primary';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je ne pense pas que le composant devrait avoir a gérer tous les thèmes différents, ça ne me parait pas scalable.

Je vois deux façon de faire qui pourrai éviter au composant de gérer ça lui-même.

  1. Avoir une prop className?: string sur ton composant Card ce que tu fais déjà.
    Ensuite pour styliser le composant de l'extérieur tu pourrais faire :
const PrimaryCard = styled(Card)`
  background: #6a8a83;
  color: white;
`

Mais ça demande à avoir une connaissance de la structure du composant ce qui brise un peu le principe d'encapsulation.
2. Définir des tokens modifiables de l'extérieur (variable CSS). Par exemple:

const Container = styled.div`
  padding: 20px;
  width: 325px;
  border-radius: 4px;
  box-shadow: 0px 2px 4px 0px rgba(0, 0, 0, 0.24);
  color: var(--card-color);
  background-color: var(--card-background-color);
`;

Et le styliser comme la solution 1 mais en spécifiant juste la valeur de ces tokens:

const PrimaryCard = styled(Card)`
  --card-background-color: #6a8a83;
  --card-color: white;
`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants