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

rewrite preview component to hooks #160

Merged
merged 1 commit into from
Dec 3, 2019
Merged

Conversation

DushanT
Copy link

@DushanT DushanT commented Nov 29, 2019

second try, component in hooks not dependent on other PR

isCodeShown: false,
isInteractive: false,
previewBackground: this.props.bgThemeColors
const getBackgroundsAsArray = (previewBackgrounds, excludedColors = []) =>

Choose a reason for hiding this comment

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

Preco sa telo tejto funkcie prepisovalo z .filter.map na .reduce?

Choose a reason for hiding this comment

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

prislo mi rozumnejsie prechadzat pole iba raz a rovno to tam checkovat

Choose a reason for hiding this comment

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

btw neviem ci to bolo spravne ale aspon to pomohlo v tom ze beni mi povedal ze konecne pochopil co ten reduce v skotocnosti robi a ako ho pouzivat.

Choose a reason for hiding this comment

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

Akoze na pochopenie reducue super :) Ale teda performance zlepsenie v tejto situacii je naozaj minimalny a zanedbatelny a myslim ze .filter.map je viac citatelne, co je sktutocny benefit v tejto situacii.

Copy link
Author

Choose a reason for hiding this comment

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

je pravda že ak niekto nepozná reduce je to menej čitateľné. ale mne príde reduce ako veľmi dobrý nástroj na zredukovanie poľa podľa nejakých podmienok. asi som ho už využíval veľa tak som si zvykol. určite je možné že pre niekoho to bude čitateľnejšie :) bol to len môj úlet asi.

const toReneder = childrenToRender || (
// eslint-disable-next-line react/no-danger
<div dangerouslySetInnerHTML={{ __html: html }} />
// TODO zjednotit

Choose a reason for hiding this comment

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

Zjednotit co? :)

Choose a reason for hiding this comment

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

su tu tri velmi podobne if statementy na to sme zabudli :D

Choose a reason for hiding this comment

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

a co je s nimi zle? :)

Copy link
Author

Choose a reason for hiding this comment

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

nič :D ale opakovanie troch identických vecí hneď za sebou mi rozsvieťuje kontrolku v hlave že niečo nie je programátorsky správne :) ale kým sú za sebou tak je to aspoň prehľadné. keby boli v troch rôznych fileoch už by to bolo menej prehľadné pri refaktoringu.

@adammockor adammockor merged commit b20b810 into master Dec 3, 2019
@adammockor adammockor deleted the component/preview-hooks branch December 3, 2019 15:08
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.

None yet

3 participants