refactor: add latest bore and allow ts declaration generation #157
Conversation
|
|
||
| import { ColorType, cssClassForColorType, css } from '../_helpers'; | ||
| import { | ||
| ColorType, |
There was a problem hiding this comment.
Just thinking - what about change order of imports to first helpers, then types? like:
import { cssClassForColorType, css, ColorType, IntrinsicCustomElement, IntrinsicBoreElement, ClickEvent } from '../_helpers'
There was a problem hiding this comment.
hmm thats too much nit picking IMHO. But I see your point. We are importing a lot of stuff from helpers but that's what is it for.
Important is to always order imports in order
- 3rd party
- local ( non package related ) - this should go away when we have proper build
- package internal relative imports
Maybe we can extract this definitions to root /packages/definitions.ts
Also that _helpers folder is wrong, it should be separate package named core or base
| }); | ||
|
|
||
| it( `should render`, () => { | ||
| return mount( <Button /> ).wait(( element ) => { |
There was a problem hiding this comment.
shouldn't we use <bl-button> here? I know that it's stated here https://github.com/wc-catalogue/blaze-elements/blob/master/docs/styleguides/TESTING.md#example--button- but I did forgot why.
There was a problem hiding this comment.
shouldn't we use here?
use what?
There was a problem hiding this comment.
ups sorry forgot back ticks around the tag
There was a problem hiding this comment.
👋 still waiting for an answear
There was a problem hiding this comment.
don't remember either ¯_(ツ)_/¯
maybe few suggestions about your review attitude:
- if you intend to stop merging things because some unrelated unanswered questions, that's very unfortunate for velocity and for everyone that's participating on this project
- we can change it ofc in another PR which should refactor other tests that have the same pattern and update the docs accordingly
There was a problem hiding this comment.
then please fix it to <bl-button>
thanks
p.s. this tone of suggestion is really unfortunate, point of PR is answering questions. PR comments are not the right place for discussing attitudes and any PR unrelated topic. Please contact me directly if you want to discuss this further. Thanks
There was a problem hiding this comment.
Can you pls unblock merging of this PR. You are blocking rest of the team. thx
packages/button/Button.tsx
Outdated
| type ButtonProps = Props & EventProps; | ||
| type Props = { | ||
| export type ButtonProps = Props & Attrs & EventHandlers; | ||
| export type Attrs = { |
There was a problem hiding this comment.
we could probably have everything in Props as all props will have attribute:{source: true} as default right? Or is there any reason to explicitly state it.
There was a problem hiding this comment.
it depends on Component ofc.
Button "primary" api is via attributes, that is also what is tested in test. Also this "clearly" separates type definitions to common pattern.
Props - primary API, should be preferred over attributes whenever possible
Attrs - only if primary API is via attributes,
Events - event aliases which we emit, specifically for bore h
EventHandlers - event aliases which we emit, specifically for IntrinsicElement consumption within other Virtual Dom view renderers
Attrs and Events are specifically for bores h while intersection of all 3 is our intrinsic element API.
We can discuss it further ofc, this is just what I ended up with, after evaluation of all possible scenarios
There was a problem hiding this comment.
yes I got the idea but I'm not sure what does it mean
only if primary API is via attributes
in reality. How you decide that that particular property is primarily attribute? Also I though that we won't test attributes as that's functionality which is tested in skatejs and in future it will be by default with attribute:{source: true}.
Let's discuss this with rest directly wdyt @hopkins-tk @davidrus?
| @@ -3,6 +3,7 @@ | |||
| "version": "1.2.0", | |||
| "main": "dist/main.js", | |||
| "license": "MIT", | |||
| "typings": "definitions/index.d.ts", | |||
There was a problem hiding this comment.
please add it also to template https://github.com/wc-catalogue/blaze-elements/blob/master/toolbelt/templates/package.json
There was a problem hiding this comment.
this is up to discussion, because current build is non existent till #153
There was a problem hiding this comment.
well template should reflect desired state for each package, also it doesn't have to do anything with our current build if definitions are not working it shouldn't be here as well.
Simply I want to have package.json to be auto generated to some degree so we can easily change tooling for all packages.
There was a problem hiding this comment.
My comment was about the path of typings within package.json
There was a problem hiding this comment.
| @@ -1,5 +1,9 @@ | |||
| { | |||
| "extends": "../../tsconfig.json", | |||
| "compilerOptions": { | |||
Hotell
force-pushed
the
mh/new-bore-and-ts-declaration
branch
from
c202710
to
57f6eaf
Compare
Hotell
changed the title
| }); | ||
|
|
||
| it( `should render`, () => { | ||
| return mount( <Button /> ).wait(( element ) => { |
There was a problem hiding this comment.
👋 still waiting for an answear
Hotell
force-pushed
the
mh/new-bore-and-ts-declaration
branch
from
57f6eaf
to
4fa8a55
Compare
No description provided.