Skip to content

Canvas annotation app #407

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

Merged
merged 25 commits into from
Mar 19, 2020
Merged

Canvas annotation app #407

merged 25 commits into from
Mar 19, 2020

Conversation

ycaokris
Copy link
Contributor

@ycaokris ycaokris commented Feb 10, 2020

Issue for app: #https://github.com/plotly/dash-customer-success/issues/255

App pull request

  • This is a new app
  • I am improving an existing app (redesigns/code "makeovers")

About

Workflow

  • I have created a branch in the appropriate monorepo, and the
    elements necessary for successful deployment are in place.
  • If the app is a redesigned and/or restyled version of an
    existing gallery app, I've summarized the changes requested in the
    appropriate Streambed issue and confirm that they have been applied.
  • If the app is on the Dash Gallery portal, I have added a link to
    the GitHub repository for the source code in the portal description.
  • If the app is a reimplementation of a Python gallery app for the
    DashR gallery, the app in this PR mimics, as closely as possible,
    the style and functionality of the existing app.=
  • I have removed all Google Analytics code from the app's
    assets/ folder.

The pre-review review

I have addressed all of the following questions:

  • Does everything in my code serve some purpose? (I have removed
    any dead and/or irrelevant code.)
  • Does everything in my code have a clear purpose? (My code is
    readable and, where it isn't, it has been commented appropriately.)]
  • Am I reinventing the wheel? (I have used appropriate packages to
    lessen the volume of code that needs to be maintained.)

@ycaokris ycaokris changed the title iniital app commit (wip) Canvas annotation app Feb 12, 2020
@ycaokris ycaokris marked this pull request as ready for review February 12, 2020 22:42
Copy link
Contributor

@christopherjeon christopherjeon left a comment

Choose a reason for hiding this comment

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

Is it okay if the layout doesn't resize properly when the window size decreases?

Screen Shot 2020-02-28 at 5 45 48 PM

Apart from that minor issue, 💃

@ycaokris
Copy link
Contributor Author

ycaokris commented Mar 4, 2020

Is it okay if the layout doesn't resize properly when the window size decreases?

Screen Shot 2020-02-28 at 5 45 48 PM

Apart from that minor issue, 💃

Good catch. I'll need a release of dash Canvas for implementing responsiveness. Once this PR is merged plotly/dash-canvas#39

@lundstrj
Copy link
Contributor

@sstripps1 you worked on this, right? Looks like. this got stuck in code review.
What is your take, should this get merged as is or should we dive in and examine this app more closely? My understanding is that this was more of a tech demo we did for a specific occasion, is that correct?

@sstripps1
Copy link
Contributor

@lundstrj this app is good to go other than a mobile responsiveness bug, which will be resolved once plotly/dash-canvas#39 is merged and we can use an updated version of canvas. Although we could probably merge for now (since it's mostly just for demo purposes anyway) and then add in that fix at a later date.

@lundstrj
Copy link
Contributor

okay, cool. Someone will have to run black on this and then we'll merge then.

I suppose I could do that. unless... @AirballClaytonCalvin

@AirballClaytonCalvin
Copy link
Contributor

Happy to 'run black on this'. Although not 100% sure what that means.

@sstripps1
Copy link
Contributor

@AirballClaytonCalvin I can walk you thru it 💯

@AirballClaytonCalvin
Copy link
Contributor

@sstripps1 I ran Black and it made 1 minor change to the app.py file. However, when I run it locally after the update, I get a callback error in my browser.

'Callback error updating text-output.children'

I am struggling to troubleshoot it right now.

@AirballClaytonCalvin AirballClaytonCalvin self-requested a review March 19, 2020 18:52
@AirballClaytonCalvin
Copy link
Contributor

💃

Copy link
Contributor

@AirballClaytonCalvin AirballClaytonCalvin left a comment

Choose a reason for hiding this comment

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

💃

Copy link
Contributor

@sstripps1 sstripps1 left a comment

Choose a reason for hiding this comment

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

💃 good to merge

@AirballClaytonCalvin AirballClaytonCalvin merged commit 639ebbb into master Mar 19, 2020
@AirballClaytonCalvin AirballClaytonCalvin deleted the oss-canvas-text branch March 19, 2020 19:03
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.

5 participants