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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/use grid layout #25

Merged
merged 10 commits into from
Mar 4, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 7 additions & 18 deletions public/app/app.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,26 +17,15 @@ var NotFoundRoute = Router.NotFoundRoute;
var Redirect = Router.Redirect;
var Link = Router.Link;

var MainScreen = require('./screens/main').MainScreen;
var DevicesRegion = require('./screens/main').DevicesRegion;
var SettingsRegion = require('./screens/main').SettingsRegion;

var ControlCenter = React.createClass({
render: function () {
return (
<RouteHandler/>
)
}
});
var MainScreen = require('./screens/main');
var DevicesRegion = require('./screens/main/sections/devices');
var SettingsRegion = require('./screens/main/sections/settings');

var routes = (
<Route path="/" handler={ControlCenter}>
<Route name="main" handler={MainScreen}>
<Route name="settings" handler={SettingsRegion}/>
<Route name="devices" handler={DevicesRegion}/>
<Redirect from="/" to="devices"/>
</Route>
<Redirect from="/" to="main"/>
<Route path="/" handler={MainScreen}>
<Route name="settings" handler={SettingsRegion}/>
<Route name="devices" handler={DevicesRegion}/>
<Redirect from="/" to="devices"/>
</Route>
Copy link
Contributor

Choose a reason for hiding this comment

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

You decided to get rid of subrouting for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, there's currently no need for it. Can be added if necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, anyway we do know how it works.

);

Expand Down
28 changes: 0 additions & 28 deletions public/app/common/regions/navigation/navigation.jsx

This file was deleted.

13 changes: 0 additions & 13 deletions public/app/common/regions/navigation/style.scss

This file was deleted.

4 changes: 3 additions & 1 deletion public/app/project-setup/theme/_base.scss
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Items should not be rearranged as the order has implications
@import "foundation/components/global";
@import "foundation/components/type";
@import "foundation/components/grid";
@import "foundation/components/side-nav";
@import "foundation/components/buttons";
@import "foundation/components/panels";
@import "foundation/components/icon-bar";
@import "foundation/components/visibility";
7 changes: 0 additions & 7 deletions public/app/project-setup/theme/_variables.scss

This file was deleted.

8 changes: 7 additions & 1 deletion public/app/project-setup/theme/variables/_base.scss
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,10 @@ $include-html-global-classes: true;

// this is used for Headers
$include-html-type-classes: true;
$include-html-grid-classes: true;

$include-html-grid-classes: false;

// this is used as extensions for sass-based styles
$include-html-visibility-classes: true;

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

$include-html-icon-bar-classes: true;
2 changes: 1 addition & 1 deletion public/app/project-setup/theme/variables/_grid.scss
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
@import "foundation/functions";

$row-width: rem-calc(1000);
$row-width: 1000px;
Copy link
Contributor

Choose a reason for hiding this comment

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

1000px is a requirement from design guys?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

$total-columns: 20;
$column-gutter: rem-calc(0);
14 changes: 0 additions & 14 deletions public/app/screens/main/devices/device/device.jsx

This file was deleted.

5 changes: 0 additions & 5 deletions public/app/screens/main/devices/device/style.scss

This file was deleted.

55 changes: 30 additions & 25 deletions public/app/screens/main/main-screen.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,35 +3,40 @@ var React = require("react");
var Router = require('react-router');
var RouteHandler = Router.RouteHandler;

var Navigation = require('common/regions/navigation');
var navigationStore = require('stores/navigation')
var Navigation = require('./navigation');
var navigationStore = require('stores/navigation');
var StyleMixin = require('mixins/style-mixin');

var style = require("./style.scss");

var MainScreen = React.createClass({

getInitialState: function() {
return {
navItems: navigationStore.getItems()
}
},
mixins: [StyleMixin(require('./style.scss'))],

render: function () {
return (
<div className="main-screen">
<div className="main-screen__navigation">
<Navigation items={this.state.navItems}/>
</div>
<div className="main-screen__content">
<RouteHandler/>
</div>
</div>
)
}
getInitialState: function () {
return {
navItems: navigationStore.getItems()
}
},

render: function () {
var navigation = <Navigation items={this.state.navItems} orientation="medium-vertical small-horizontal"/>;

return (
<div className="main-screen">
<div className="main-screen__top-row main-screen--visible-for-small-only">
{navigation}
</div>
<div className="main-screen__content-row">
<div className="main-screen__sidebar-column main-screen--hidden-for-small">
{navigation}
</div>
<div className="main-screen__content-column">
<RouteHandler/>
</div>
</div>
</div>
)
}
});

module.exports = {
MainScreen: MainScreen,
DevicesRegion: require('./devices'),
SettingsRegion: require('./settings')
};
module.exports = MainScreen;
30 changes: 30 additions & 0 deletions public/app/screens/main/navigation/navigation.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
'use strict';

var React = require('react'),
Link = require('react-router').Link,
StyleMixin = require('mixins/style-mixin');

var Navigation = React.createClass({
mixins: [StyleMixin(require("./style.scss"))],
render: function () {
// TODO: Use mixins instead of CSS classes
// TODO: Pass number of elements to mixin instead of specifying class name
var classes = "cc_navigation icon-bar two-up " + this.props.orientation;
Copy link
Contributor

Choose a reason for hiding this comment

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

Classes icon-bar two-up should be included by cc_navigation? This could however by done by a different PR that concentrates on using mixins for this icon-bar


var items = this.props.items.map(function (item, i) {
return (
<Link to={item.linkTo} key={i} className="cc_navigation__link item">
Copy link
Contributor

Choose a reason for hiding this comment

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

class item should be included by mixin class cc_navigation__link?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need key here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. The answer is the same as to the same question you did recently. It's needed by react: https://coderwall.com/p/jdybeq/the-importance-of-component-keys-in-react-js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@larsblumberg two comments:

  1. item should be fixed in a separate scope, as it is not a part of the template
  2. key is not needed, if you are ok with the array indices used as keys. React do that automatically (Consider providing a default key for dynamic children facebook/react#1342 (comment))

Copy link
Contributor

Choose a reason for hiding this comment

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

Not defining the key property will however fire a warning onto the console. This is for good reason as the loop variable might not always be a good key (see the blog entry). This said I would prefer to always define a key explicitly so we never forget it.

<label>{item.title}</label>
</Link>
);
});

return (
<div className={classes}>
{items}
</div>
);
}
});

module.exports = Navigation;
12 changes: 12 additions & 0 deletions public/app/screens/main/navigation/style.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
@import "theme";

.cc_navigation {
@include icon-bar();

.cc_navigation__link {
// .active is applied to selected link by foundation automatically
&.active {
font-weight: bold;
}
}
}
19 changes: 19 additions & 0 deletions public/app/screens/main/sections/devices/device/device.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
'use strict';
var React = require('react');
var StyleMixin = require('mixins/style-mixin');

var Device = React.createClass({
mixins: [StyleMixin(require('./style.scss'))],
render: function () {
return (
<div className="cc-devices__device row">
<div className="cc-devices__device--title small-6 column">{this.props.device.title}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we move small-6 to mixin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is NOT a part of template scope - it should be addressed when dealing with device component

<div className="cc-devices__device--type small-6 column">{this.props.device.type}</div>
<div className="cc-devices__device--control small-4 column">Control</div>
<div className="cc-devices__device--state small-4 column">{this.props.device.state}</div>
</div>
);
}
});

module.exports = Device;
10 changes: 10 additions & 0 deletions public/app/screens/main/sections/devices/device/style.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
@import "theme";

.cc-devices {
.cc-devices__device {
.cc-devices__device--title {
font-weight: bold;

}
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
var Device = require('screens/main/devices/device/device.jsx');
var Device = require('../device.jsx');

var React = require('react/addons'),
TestUtils = React.addons.TestUtils;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@ var Room = require('./room');
var roomStore = require('stores/room');
var roomActions = require('actions/room');

require('./style.scss');
var StyleMixin = require('mixins/style-mixin');

// TODO: Rename to DevicesSection
var DeviceRegion = React.createClass({
mixins: [Reflux.connect(roomStore, "rooms")],
mixins: [
Reflux.connect(roomStore, "rooms"),
StyleMixin(require('./style.scss'))
],

getInitialState: function() {
return {rooms: roomStore.getRooms()};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
'use strict';

var React = require('react');
var Device = require('screens/main/devices/device/index');
var Device = require('../device');

require('./style.scss');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why StyleMixin isn't used here?

Copy link
Contributor

Choose a reason for hiding this comment

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

was not part of the PR, we worked on the main screen and navigation bar. This could be done by a separate PR


var Room = React.createClass({
render: function () {
Expand Down
38 changes: 32 additions & 6 deletions public/app/screens/main/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,38 @@

.main-screen {
@include grid-row();
height: 100%;
}

.main-screen__top-row-small-only {
@include grid-row();
}

.main-screen__content-row {
@include grid-row();
height: 100%;
}

.main-screen__navigation {
@include grid-column(4);
}

.main-screen__content {
@include grid-column(8);
.main-screen__sidebar-column {
@include grid-column(6);
height: 100%;
}

.main-screen__content-column {
@include grid-column(20);
background-color: $shade-gray;
height: 100%;

@media #{$medium-up} {
@include grid-column(14);
}
}
}

.main-screen--hidden-for-small {
@extend .hide-for-small-only;
}

.main-screen--visible-for-small-only{
@extend .visible-for-small-only
}
6 changes: 3 additions & 3 deletions public/app/stores/room/room-store.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ var roomActions = require('actions/room');

var _rooms = [
{title: 'Living room', devices: [
{title: 'yetu Home Gateway'},
{title: 'Nest'}
{title: 'yetu Home Gateway', type: 'Home Gateway', state: 'connected'},
{title: 'Nest', type: 'Thermostat', state: 'not conn.'}
]},
{title: 'Bed room', devices: [
{title: 'Nest'}
{title: 'Nest', type: 'Thermostat', state: 'connected'}
]},
{title: 'Bath room', devices: []}
];
Expand Down