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

Implement buttons #77

Merged
merged 3 commits into from
Nov 17, 2020
Merged

Conversation

fidelman
Copy link

@fidelman fidelman commented Oct 26, 2020

Closes #65

Dokoncit:

Tlacitko

  • Jednoduchy komponent
  • Jednoduche styly se zavislostmi
  • Pokryt vsechny pseudo-classy

Odpad

Storybook

  • opravit storybook chyby
  • pridat tlacitko k storybooku
  • pridat odpad k storybooku

@vercel

This comment has been minimized.

@fidelman

This comment has been minimized.

@Erbenos

This comment has been minimized.

@fidelman

This comment has been minimized.

.storybook/main.js Outdated Show resolved Hide resolved
.storybook/webpack.config.js Outdated Show resolved Hide resolved
@mzadrazi

This comment has been minimized.

@fidelman fidelman force-pushed the implement-buttons branch 2 times, most recently from 1b2c3a5 to 19022e9 Compare November 1, 2020 20:13
@fidelman fidelman requested a review from HormCodes November 1, 2020 20:19
@fidelman fidelman changed the title WIP Implement buttons Implement buttons Nov 1, 2020
@fidelman

This comment has been minimized.

@Haaxor1689

This comment has been minimized.

src/components/link-as-button/index.tsx Outdated Show resolved Hide resolved
src/components/link/styles.ts Outdated Show resolved Hide resolved
@Haaxor1689

This comment has been minimized.

@fidelman

This comment has been minimized.

@Haaxor1689

This comment has been minimized.

Haaxor1689 added a commit to Haaxor1689/web that referenced this pull request Nov 3, 2020
@Haaxor1689 Haaxor1689 mentioned this pull request Nov 3, 2020
9 tasks
Copy link
Contributor

@HormCodes HormCodes left a comment

Choose a reason for hiding this comment

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

Andreii, za mne dobře odvedená práce a díky za to. 👍

Mám k tomu pár bodů, jak si myslím, že by se to dalo vylepšit:

Generování stylů na základě props
Úplně se mi nelíbí používání ternárních operátorů a zamyslel bych se nad level of abstraction. Sám jsem to prakticky nezkoušel, ale líbilo by se mi mít samostatné funkce vracející CSS pro konkrétní stylové skupiny - např. font, border, barvy atd.

V podstatě si představuji takovéto funkce:

function getBorderStyle(props: StyledLinkProps): FlattenInterpolation<ThemeProps<DefaultTheme>> {
  if (props.disabled) {
      // ...
  }
  
  // Default case
  return css``
}

Co si o tom myslíš?

Poprosil bych také o extrakci všech magických konstant pro barvy.

Malé drobnosti

  • Stories bych měl všechny pro komponenty, ať je viditelné, že komponenta existuje
  • Pro přehlednost bych i základní props vycházející z HTML elementu ( např. onClick u tlačítka)

To je prozatím vše. 🙂

@fidelman

This comment has been minimized.

@HormCodes

This comment has been minimized.

@fidelman

This comment has been minimized.

@cesko-digital cesko-digital deleted a comment from fidelman Nov 11, 2020
@vercel
Copy link

vercel bot commented Nov 12, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/ceskodigital/web/ep6a8vk1w
✅ Preview: https://web-git-implement-buttons.ceskodigital.vercel.app

@HormCodes
Copy link
Contributor

Ahoj všichni,
@fidelman nám odjel na dovolenou, takže jsem PR od něj přebral a vzal si na starost dokončení.

Musím říct, že bylo a je pro mne obtížné se občas v komunikaci orientovat (to pak i zapříčinilo mé zpoždění se vším) a proto jsem se rozhodl většinu komentářů schovat a popřemýšlím nad nastavením nějakých pravidel, ať je to pro nás všechny přehledné. 😉

Nakonec jsem zanechal rozdělení do čtyř komponent. Pokud se to ukáže jako limitující, můžeme kdykoliv v budoucnu vytvořit issue na refactoring, ale takto mi to zatím přijde dostatečně čitelné a jednoduché na použití. Je to prozatím finální rozhodnutí pro tyto změny. 🙂

Poprosím @mzadrazi a @patrikbraborec o závěrečný pohled minimálně jednoho z vás. Pokud to bude OK, vyčistím historii a konečně zamergujeme.

@patrikbraborec
Copy link
Contributor

@HormCodes Ahoj, super. Já na to mrknu dneska večer. :)

src/components/buttons/button/styles.ts Outdated Show resolved Hide resolved
src/components/buttons/button/styles.ts Show resolved Hide resolved
src/components/buttons/button/styles.ts Outdated Show resolved Hide resolved
src/components/links/button-as-link/index.tsx Outdated Show resolved Hide resolved
src/components/links/link/styles.ts Outdated Show resolved Hide resolved
@patrikbraborec
Copy link
Contributor

@HormCodes Ahoj, jednou jsem to proletěl a mám tam pár drobností, tak se mrkni 🙂

A ještě jen drobnost: před mergem bych rozdělil na dva commity, jelikož jsou tam nějaké změny v CONTRIBUTING.md 🙂

@HormCodes HormCodes merged commit 8f65083 into cesko-digital:rework Nov 17, 2020
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.

6 participants