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

Fix tests, small changes #1

Merged
merged 5 commits into from
Jun 8, 2020
Merged

Fix tests, small changes #1

merged 5 commits into from
Jun 8, 2020

Conversation

luwes
Copy link

@luwes luwes commented Jun 8, 2020

@heyheyhello

I fixed up a test, api.hs is used in some other packages and I think I would prefer to keep that uniform to api.h both of which could be overriden.

svgJSX is good but I'm not sure if it should be in the core.
wouldn't this be a good utility function in the codebase of the user?

also is the api.s counting needed for nesting?
I might need to change the api.hs function then


some lint errors

Screen Shot 2020-06-07 at 9 14 59 PM

@nettyso nettyso merged commit 887b348 into nettyso:svg-jsx Jun 8, 2020
@nettyso
Copy link
Owner

nettyso commented Jun 8, 2020

I rebased the accidental merge but surprisingly GitHub's not figuring it out. See luwes#114

@nettyso
Copy link
Owner

nettyso commented Jun 8, 2020

Also yes the svgJSX counting was for nesting, since the alternative to counting would be saving/restoring the previous boolean as I used to do in https://github.com/heyheyhello/sinuous/blob/16af9cd5cf1ba3b28bad9761a09d10b5f2569607/packages/sinuous/src/index.js#L43-L49

Otherwise doing something like

const OtherPath = () => svg`<path .../>`
const MyComp = () =>
  svg`
    <svg ....>
      <path fill-rule="evenodd" d="M8 2.748l.../>
      <${OtherPath}/>
      <title>Is this going to be an SVG tooltip or set the HTML page's tab title?</title>
    </svg>
  `

When OtherPath is reached svg`` will turn off api.s and then continue doing work in MyComp in HTML mode?... I haven't checked, it's late and I'm supposed to be studying - might be confused about order of operations. Maybe you could introduce a small test case to make sure it doesn't break anything

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