-
Notifications
You must be signed in to change notification settings - Fork 513
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Create Jupyter widget for Captum Insights #124
Conversation
Awesome! I'll give it a more thorough review tomorrow, can you check why the tests are failing? |
e59abb1
to
8d5c707
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic! Thanks for working on this. Looks good to me.
Can you format your Javascript code using Prettier?
captum/insights/frontend/src/App.css
Outdated
button { | ||
cursor: pointer; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why leave this here? should we just move it all into the css module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the main purpose of CSS modules is to allow locally scoped styles linked to classnames. For that reason, I put all class-specific styles in the App.module.css file and all global styles in App.css. A CSS selector like this isn't directly accessible by CSS modules since there's no classname tied to it.
Example: if we import the CSS modules like so:
import styles from "./App.module.css";
You can't refer to styles.button
, but you can use styles.btn
since .btn is a classname.
That being said, global styles like button { ... } will still work as expected when placed in a CSS modules file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should move this into the .btn class, does it influence the Jupyter notebook at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the Jupyter notebook buttons are already styled to have cursor: pointer;
, but I think it would be more proper to scope this locally in the .btn
class.
a6d0139
to
b8f504c
Compare
Formatted JS code with Prettier and moved WebApp react component to its own file. |
Rebase to master and resolve the merge conflicts please :) Otherwise, LGTM. |
Create Jupyter widget for Captum Insights
serve
the webapp orrender
the widget