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

Adds "End Shape" Prop & Introduces "Circle" Option #111

Merged
merged 3 commits into from
Oct 2, 2020
Merged

Adds "End Shape" Prop & Introduces "Circle" Option #111

merged 3 commits into from
Oct 2, 2020

Conversation

damonbauer
Copy link
Contributor

@damonbauer damonbauer commented Sep 8, 2020

This PR does 2 things:

  1. Moves the arrowLength and arrowThickness props into a new endShape prop
  2. Adds a circle option to the endShape prop (and creates the ability to add additional shapes in the future)

This PR addresses #39. It also paves the way for #79 (we could introduce a startShape object).

Note: This would be a major version change as I've changed the API.

Current Implementation

<ArcherElement
  id="root"
  relations={[
    {
      ...
      style: {
        strokeDasharray: "5,5",
        arrowLength: 10,
        arrowThickness: 20
      }
    }
  ]}
>
  <div style={boxStyle}>Root</div>
</ArcherElement>;

New/Updated Implementation

<ArcherElement
  id="root"
  relations={[
    {
      ...
      style: {
        strokeDasharray: "5,5",
        endShape: {
          arrow: {
            arrowLength: 10,
            arrowThickness: 20
          }
        }
      }
    }
  ]}
>
  <div style={boxStyle}>Root</div>
</ArcherElement>;

New/Updated Implementation (with circle)

<ArcherElement
  id="root"
  relations={[
    {
      ...
      style: {
        strokeDasharray: "5,5",
        endShape: {
          circle: {
            radius: 5,
            strokeWidth: 1,
            strokeColor: 'tomato',
            fillColor: '#c0ffee'
          }
        }
      }
    }
  ]}
>
  <div style={boxStyle}>Root</div>
</ArcherElement>;

Screenshots

Screen Shot 2020-09-08 at 7 08 15 AM

src/ArcherContainer.js Show resolved Hide resolved
src/ArcherContainer.js Show resolved Hide resolved
src/ArcherContainer.js Show resolved Hide resolved
src/ArcherContainer.js Outdated Show resolved Hide resolved
@damonbauer damonbauer marked this pull request as ready for review September 8, 2020 12:27
@jfo84
Copy link
Contributor

jfo84 commented Sep 8, 2020

This looks awesome.

@pierpo
Copy link
Owner

pierpo commented Sep 15, 2020

Hey! This looks brilliant!

I'll need some time to review it. Sorry for not being very active at the moment.

Thank you for the great work 😊

@pierpo
Copy link
Owner

pierpo commented Sep 15, 2020

I just read your whole PR, I don't have much to add. This is amazing 😊

The one thing missing I can think of is the update of typescript definitions 😊

@damonbauer
Copy link
Contributor Author

@pierpo - No worries about the delay! OSS is tough - thank you for what you do!

Thank you for the feedback. I apologize for such a large PR, but I couldn't think of an easy way to break this down into small pieces without a breaking change that wasn't useful...


I've updated the TS definitions in 2ea43fa. Please let me know if you see anything missing, wrong or needing documentation, I'll be happy to address it!

Finally, is there anything you'd like me to do as far as incrementing the version, tagging the branch, etc. in order to prepare a new release?

@pierpo
Copy link
Owner

pierpo commented Sep 16, 2020

@pierpo - No worries about the delay! OSS is tough - thank you for what you do!

Thank you for the feedback. I apologize for such a large PR, but I couldn't think of an easy way to break this down into small pieces without a breaking change that wasn't useful...

I wouldn't have imagined something shorter either so no worries 😊

I've updated the TS definitions in 2ea43fa. Please let me know if you see anything missing, wrong or needing documentation, I'll be happy to address it!

Thank you!

Finally, is there anything you'd like me to do as far as incrementing the version, tagging the branch, etc. in order to prepare a new release?

No worries, I'll handle this. I'll try to do it in the following days!

@pierpo
Copy link
Owner

pierpo commented Sep 21, 2020

Just to give you some news: I'm quite busy at the moment, I'll get back to it probably next week.

Thank you for the great work and for your patience 😊

@pierpo
Copy link
Owner

pierpo commented Oct 2, 2020

Sorry I still haven't found the time (despite my promise). I really want to publish this next week!

I'll merge in the mean time so that any further contribution includes your changes.

Thank you for the amazing job I really appreciate the help 😊

@pierpo pierpo merged commit 6eff86a into pierpo:master Oct 2, 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.

None yet

3 participants