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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

new(annotations/EditableAnnotation): add canEditSubject canEditLabel #919

Merged
merged 4 commits into from Nov 23, 2020

Conversation

williaster
Copy link
Collaborator

馃殌 Enhancements

This PR adds the ability to specify whether one or both of the annotation subject or label are editable within EditableAnnotation, to support different use cases. Updates the demo to show this functionality.

Nov-05-2020 18-57-36

@kristw @hshoff

@williaster williaster added this to the 1.1.1 milestone Nov 6, 2020
@williaster williaster added this to Reviewable in XYChart via automation Nov 6, 2020
Copy link
Member

@hshoff hshoff left a comment

Choose a reason for hiding this comment

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

Nice! looks good to me, except for a minor suggestion and a quick fix for the build

onTouchStart={subjectDrag.dragStart}
onTouchMove={subjectDrag.dragMove}
onTouchEnd={subjectDrag.dragEnd}
cursor={subjectDrag.isDragging ? 'grabbing' : 'grab'}
Copy link
Member

Choose a reason for hiding this comment

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

Would we want this cursor state to configurable? maybe through subjectDragHandleProps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep! {...subjectDragHandleProps} is spread below this line so it should override cursor (tho currently it doesn't have access to subjectDrag state, is that what you meant?)

onTouchStart={labelDrag.dragStart}
onTouchMove={labelDrag.dragMove}
onTouchEnd={labelDrag.dragEnd}
cursor={labelDrag.isDragging ? 'grabbing' : 'grab'}
Copy link
Member

Choose a reason for hiding this comment

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

Same as above


describe('<EditableAnnotation />', () => {
function setup() {
function setup(props?: EditableAnnotationProps) {
Copy link
Member

Choose a reason for hiding this comment

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

Partial<EditableAnnotationProps>

@coveralls
Copy link

Pull Request Test Coverage Report for Build 371265698

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 60.853%

Totals Coverage Status
Change from base Build 348549012: 0.04%
Covered Lines: 1630
Relevant Lines: 2533

馃挍 - Coveralls

@williaster
Copy link
Collaborator Author

gonna merge this, happy to address any remaining concerns in another PR

@williaster williaster merged commit e40edb9 into master Nov 23, 2020
XYChart automation moved this from Reviewable to Done Nov 23, 2020
@williaster williaster deleted the chris--annotations-edit-labelsubject branch November 23, 2020 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
XYChart
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants