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

React dashboards #1550

Merged
merged 9 commits into from Aug 14, 2015

Conversation

Projects
None yet
2 participants
@edmundoa
Member

edmundoa commented Aug 6, 2015

Port of the dashboard list page to React. I had to do some yak-shaving in the DashboardStore, as we need to expose the list of editable dashboards in other components.

edmundoa added some commits Aug 6, 2015

Add listDashboards call into dashboards store
Also rename some members of the store to clarify what they do.
Improve EditDashboardModal component
- Clear form after dashboard creation.
- Only use jQuery to update title and description if they are present on
  the DOM.

@edmundoa edmundoa added this to the 1.2.0 milestone Aug 6, 2015

@dennisoelkers dennisoelkers self-assigned this Aug 10, 2015

const EditDashboardModalTrigger = require('./EditDashboardModalTrigger');
const PermissionsMixin = require('../../util/PermissionsMixin');

const DashboardStore = require('../../stores/dashboard/DashboardStore');

This comment has been minimized.

@dennisoelkers

dennisoelkers Aug 11, 2015

Contributor

Could you please use module imports? That way imports are theoretically independent of the module loader user.

This comment has been minimized.

@edmundoa

edmundoa Aug 11, 2015

Member

Makes sense, will change that 👍

const DashboardStore = require('../../stores/dashboard/DashboardStore');

const Dashboard = React.createClass({
mixins: [PermissionsMixin],

This comment has been minimized.

@dennisoelkers

dennisoelkers Aug 11, 2015

Contributor

Could you please add propTypes to the component? This way components are way easier to reuse.

This comment has been minimized.

@edmundoa

edmundoa Aug 11, 2015

Member

I wanted to add them, but then forgot about them. Will fix that!

},
});

module.exports = Dashboard;

This comment has been minimized.

@dennisoelkers

dennisoelkers Aug 11, 2015

Contributor

Could you please use module exports? That way exports are theoretically independent of the module loader user.

This comment has been minimized.

@edmundoa

edmundoa Aug 11, 2015

Member

Sounds good :)

<fieldset>
<div className="form-group">
<label htmlFor={`${this.props.id}-title`}>Title:</label>
<input id={`${this.props.id}-title`}

This comment has been minimized.

@dennisoelkers

dennisoelkers Aug 11, 2015

Contributor

Why don't we use the react-bootstrap components for this? Would make the code much cleaner imo.

This comment has been minimized.

@edmundoa

edmundoa Aug 11, 2015

Member

Simply because I copy-pasted from another component and didn't replace it. Will use react-bootstrap there.

This comment has been minimized.

@dennisoelkers

dennisoelkers Aug 12, 2015

Contributor

Cool, thanks! 👍

@edmundoa

This comment has been minimized.

Member

edmundoa commented Aug 12, 2015

All issues should be addressed with the latest changes 😄

dennisoelkers added a commit that referenced this pull request Aug 14, 2015

@dennisoelkers dennisoelkers merged commit 5bb9c07 into master Aug 14, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@edmundoa

This comment has been minimized.

Member

edmundoa commented Aug 14, 2015

Thank you for reviewing @dennisoelkers! 👍

@edmundoa edmundoa deleted the react-dashboards branch Aug 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment