-
Notifications
You must be signed in to change notification settings - Fork 14k
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
"Add Slices" modal on dashboard page #678
Conversation
Coverage decreased (-0.2%) to 81.084% when pulling bc2330ea67b08f17deb6d20eb65570ba24ab0adb on georgeke:add-slice into 4191b75 on airbnb:master. |
Coverage increased (+0.1%) to 81.431% when pulling 1e09d28a150aee82c51eeec814f107650eb47bb7 on georgeke:add-slice into 4191b75 on airbnb:master. |
LGTM on the python side, @ascott, do you have input on the React side? |
@@ -182,15 +167,148 @@ class GridLayout extends React.Component { | |||
margin={[20, 20]} | |||
useCSSTransforms={false} | |||
draggableHandle=".drag"> | |||
{this.state.sliceElements} | |||
{this.state.slices.map(function (slice) { |
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.
you could use es6 fat arrow here and you then you won't need to bind this
.
this.state.slices.map((slice) => {
// do things with slice
});
are the dark grey rows the selected slices? i wonder if we should use checkboxes here since that seems like a pattern we are using in most other tables where you can select a row. i think we could get rid of the show x entries drop down. since this is a modal, we don't want it to get to long if someone loads 20/50 entries. we could figure out what a good max height should be for the modal and then only show that number of slices and paginate the rest. |
<button type="button" className="close" data-dismiss="modal" aria-label="Close"> | ||
<span aria-hidden="true">×</span> | ||
</button> | ||
<h4 className="modal-title" id="myModalLabel">Add New Slices</h4> |
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.
is this id used for anything?
Coverage increased (+0.1%) to 81.141% when pulling 5a3a84e5d23afc785ee2b8744cb2d92a22a46660 on georgeke:add-slice into 8135c24 on airbnb:master. |
customButtons: PropTypes.node | ||
}; | ||
|
||
class Modal extends React.Component { |
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.
it would be great to move this component to caravel/assets/javascripts/components/Modal.jsx
so we can reuse it across the app.
Bumps [whatwg-fetch](https://github.com/github/fetch) from 3.0.0 to 3.2.0. - [Release notes](https://github.com/github/fetch/releases) - [Commits](JakeChampion/fetch@v3.0.0...v3.2.0) Signed-off-by: dependabot-preview[bot] <[email protected]> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Bumps [whatwg-fetch](https://github.com/github/fetch) from 3.0.0 to 3.2.0. - [Release notes](https://github.com/github/fetch/releases) - [Commits](JakeChampion/fetch@v3.0.0...v3.2.0) Signed-off-by: dependabot-preview[bot] <[email protected]> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Bumps [whatwg-fetch](https://github.com/github/fetch) from 3.0.0 to 3.2.0. - [Release notes](https://github.com/github/fetch/releases) - [Commits](JakeChampion/fetch@v3.0.0...v3.2.0) Signed-off-by: dependabot-preview[bot] <[email protected]> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Bumps [whatwg-fetch](https://github.com/github/fetch) from 3.0.0 to 3.2.0. - [Release notes](https://github.com/github/fetch/releases) - [Commits](JakeChampion/fetch@v3.0.0...v3.2.0) Signed-off-by: dependabot-preview[bot] <[email protected]> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Notes: