[WIP] Qase v.0.1 #2
Conversation
n3r
requested review from
nl-kaidzen,
iiojib,
titovmx,
nismangulov and
ibedarev
| [SpacerJustify.SpaceBetween]: 'justify-content: space-between;' | ||
| }; | ||
|
|
||
| export const StyledSpacer = styled.div<StyledDivProps>` |
There was a problem hiding this comment.
I see a few issues with such a high degree of styled component customization:
- requires providing a lot of additional enums and mappings of keys to CSS rules. You might end up with all CSS declared in this way.
- having
$cssprop as the plain text makes it unclear which css properties should be defined as props, which should be defined as ad-hoc$cssvalue - props passing through a few components, i.e. Spacer -> StyledSpacer, you have to repeat any new prop whenever it is added.
- I guess performance is still issue of Styled Components, and all these props have to be recalculated on render and reinserted into the document. It's likely not a problem for a small app, however, could become while codebase is growing.
I would recommend simplifying such components. If you need to have div in a lot of different ways I'd rather put it as a styled div in that particular place without defining maps of styles. If you don't want to repeat a common part, you can extract it and reuse later like:
const FlexDiv = styled.div`
display: flex;
box-sizing: border-box;
`
There was a problem hiding this comment.
Yes very good point. Spacer become too large. $css prop was added mostly in order to deal with margins.
Do you suggest to create styled component for each case separatly, and then reuse most common of them?
And do you suggest to minify mapping props to styles as much as possible? For instanse use it only for colors, padding and margins?
There was a problem hiding this comment.
removed and css prop approach from codebase
src/utils/jsonp/qase-jsonp.ts
Outdated
| } | ||
| const resolve = getJsonpResolver(callbackId); | ||
| if(!resolve) { | ||
| throw new Error('No resolver was found!') |
There was a problem hiding this comment.
You have prettier in the project but seemingly it is not set up in your IDE.
I'd recommend doing it and running npx prettier 'src/**/*.{ts,tsx}' --write to auto-fix current problems.
Notice that the current .prettierrc.json dictates having 2-spaces indent and eliminating semicolons
There was a problem hiding this comment.
yes, for sure
src/utils/jsonp/jsonp-fetch.ts
Outdated
| const script = document.createElement('script'); | ||
| const callbackId = generateId(path); | ||
| let onAbort: undefined|EventListenerOrEventListenerObject; | ||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Could you please explain your motivation to use this approach to load data? Have you considered using Fetch API?
There was a problem hiding this comment.
As far as I understand. Fetch api does not support JSONP out of the box (because of CORS when you open local html file). I create this fetch-like wrapper in order to load jsonp files. It implements standard JSONP flow. Loading jsonp like it is javascript, and by implementing global jsonp loader function. Plus some logic to make this api fetch-like.
There was a problem hiding this comment.
Anyway we can get rid of this, by trying to use libs like jsonp-fetch instead.
There was a problem hiding this comment.
Just curious where did you take these icons? Have you considered using FontAwesome lib?
There was a problem hiding this comment.
I exported them from the figma, and minified by svgo after that. Didn't noticed that all of them come from font awesome lib.
There was a problem hiding this comment.
partially fixed, some components requires PRO version of lib, waiting for credentials
No description provided.