Match vision tab overhaul by micaelbergeron · Pull Request #469 · odota/web

micaelbergeron · 2016-11-11T01:50:29Z

fixes #342

This PR addresses #342 and will add a bunch of new features to the Vision Tab, including:

Dynamic ward filtering
Player/Ward type filtering
Time slider (controls visible wards on the map)
Ward log
Counterwarding (require Add a warding processor parser#3)

Keep in mind that this is a work in progress.
I've also refactored certain components while working on this page.

There are still refactoring/extracting that needs to be done.

Comments welcome.

remove the usage of redux's Connect from the vision page by using the PlainTable instead of the default Table (TableWithOptions). there are still performance issues, buut it is usable.

i've decided to not use a <table> anymore to host this data as it seems the material-ui table causes some issues with transitions and animating table row is hacky at best a simple <ul> implementation seems to be more performant

# Conflicts: # src/components/Match/Match.css # src/components/Match/VisionMap.jsx # src/components/Match/matchColumns.jsx # src/components/TabBar/TabBar.jsx # src/components/Table/Table.css # src/components/Table/Table.jsx # src/components/Visualizations/Table/HeroImage.jsx # webpack.config.js

howardchung · 2016-11-11T03:32:56Z

src/components/Match/VisionMap.jsx

-      enabledIndex: {},
-    });
+  shouldComponentUpdate(newProps) {
+    if (newProps.wardsLog.length == this.props.wardsLog.length) return false;


This is a boolean so you can return it directly

howardchung · 2016-11-11T03:33:16Z

src/components/Match/VisionMap.jsx

-      enabledIndex: {},
-    });
+  shouldComponentUpdate(newProps) {
+    if (newProps.wardsLog.length == this.props.wardsLog.length) return false;


This is a boolean so you can return it directly

howardchung · 2016-11-11T03:34:10Z

src/components/Match/VisionMap.jsx

-    this.updateMap = this.updateMap.bind(this);
+
+  componentDidMount() {
+    window.addEventListener("resize", () => this.forceUpdate());


I think @mike-robertson will advise that you use redux-responsive's state.browser.width for this

Let's iterate on this once the component is shipped.

howardchung · 2016-11-11T03:36:09Z

src/components/Match/VisionPage.jsx

+class PlayerFilter extends React.PureComponent {
+  constructor(props) {
+    super(props);
+    this.getObserverCount = () => this.props.player.obs_log.length;


i'd advise making these functions outside the class, and take props as a parameter.

This component will be refactored.

howardchung · 2016-11-11T03:36:46Z

src/components/Match/VisionPage.jsx

+            <Col xs>
+              <Button {...this.getMuiThemeProps()}
+                      label={obs_count}
+                      disabled={obs_count == 0}


use !obs_count

howardchung · 2016-11-11T03:37:27Z

src/components/Match/VisionPage.jsx

+                      backgroundColor={this.generateFilterKey("observer") in this.props.activeFilters ? filterOff : filterOn}
+                      style={{opacity: obs_count > 0 ? opacityOn : opacityOff}}
+                      onClick={() => onFilterClick(this.generateFilterKey("observer"), player.player_slot, "observer")}
+                      icon={<Avatar size={24} src="http://a19a1164.ngrok.io/apps/dota2/images/items/ward_observer_lg.png" />}


use API_HOST

Prototype left-overs.

howardchung · 2016-11-11T03:38:43Z

src/components/Match/VisionPage.jsx

+
+class VisionPage extends React.Component {
+  componentWillMount() {
+    Perf.start();


should this be disabled for production?

Isn't react-addons-perf already doing this under the hood?

Could be removed.

howardchung · 2016-11-11T03:39:35Z

src/components/Match/VisionPage.jsx

+    return _.rangeStep(interval, 0, this.props.match.duration);
+  }
+
+  _handleViewportChange(value) {


I think eslint will complain about the use of underscores since javascript doesn't really have any idea of private properties.

Yeah that was to test quickly the debounce, I'll figure out a better name

howardchung · 2016-11-11T03:40:12Z

src/components/Match/WardLog.jsx

+
+// a simple functor that will call the correct function depending on value 
+const threshold = _.curry((start, limits, values, value) => {
+  if (limits.length != values.length) throw "Limits must be the same as functions.";


I need to extract this functor in utility

howardchung · 2016-11-11T03:45:34Z

src/utility/components.jsx

+  }
+};
+
+export const Pure = (Component) => class extends React.PureComponent {


what's the difference between this and a function component?

There is no difference, it just reads better when using an existing component.

const PureRow = Pure(Row);

howardchung · 2016-11-11T03:46:03Z

src/utility/components.jsx

@@ -0,0 +1,16 @@
+import React from 'react';
+
+export const Fixed = (Component) => class extends React.Component {


Doesn't the Redux store take care of this kind of comparison already?

The Vision filters are in the local state r/n.

# Conflicts: # src/components/Match/VisionMap.jsx # src/utility/index.jsx

micaelbergeron · 2016-11-11T17:58:10Z

I must lint this.

micaelbergeron · 2016-11-12T03:48:49Z

I think this is ready to ship.

howardchung · 2016-11-12T04:04:54Z

Reviewers:
@albertcui
@blukai
@mike-robertson
@dartharaknus

blukai

Also it wont work for me VisionPage.jsx?6a5e:62 - Uncaught TypeError: Cannot assign to read only property 'key' of object '#<Object>'

blukai · 2016-11-12T10:01:40Z

src/components/Match/Match.css


+:root {
+  --ward-log-height: 50px;
+  --slider-ticks-color: #757575;


I guess this colors should be defined in palette.css

blukai · 2016-11-12T10:02:05Z

package.json

    "isomorphic-fetch": "2.2.1",
+    "lodash": "^4.16.4",
    "material-ui": "0.16.1",
+    "moment-duration-format": "^1.3.0",


Do we need this new 7 dependencies?

Let me explain why those have been added:

Lodash: I don't feel like implementing my how flow() to compose my filters. I suggest we use the 'lodash/fp' import that is more geared toward functional programming. (I also use: inRange, sortByIndex, map/filter).

moment-duration-format: I don't think I'm using it, so I guess I'll remove it. Good catch.

react-addons-css-transition-group: de facto transition component for React

react-measure: This is to deal with the resizing of the page. @howardchung suggested I use the Redux store size, but I don't see how that can integrate with the flexgrid sizing. @mike-robertson you are welcome to comment.

seamless-immutable: okay that is a subject of its own, but this manage immutable Object/Array and make it so deeply nested values can be compared with a simple reference equality check. Having immutable objects in the Redux store is considered a good practice.

react-addons-perf: That is very helpful to dig down React rendering and find performance issues

react-perf-tool: I think that expose it to the Chrome extension

blukai · 2016-11-12T10:04:55Z

src/components/Match/Match.css


+.ward-log {
+  width: 100%;
+  font-size: .8em;


We have some defined font sizes in palette.css. I guess 0.8em is somehing like 12px so u can use --fontSizeSmall

I'll oblige but I think that using px as font size is a bad practice for mobility. I'd rather leave it that way or refactor to use em/rem all around.

We should link it to --fontSizeSmall then convert that em then

This reverts commit 12d6828.

blukai

👍 Some things that we should fix someday:

team headings <Heading title.. icon../> ex: https://github.com/odota/ui/blob/master/src/components/Match/matchPages.jsx#L55
wrap this into table (maybe) and fix red

Also it can be done in this pr

mike-robertson · 2016-11-12T16:04:40Z

src/components/Match/Vision/PlayerFilter.jsx

+    const [opacityOn, opacityOff] = [1, 0.4];
+    return (
+      <Row
+        className={styles['filter-row']}


We use camelcase in the css files so we don't need to do things like this. I know camelCase isn't standard css, but for css-modules, it is.

mike-robertson · 2016-11-12T16:06:30Z

src/components/Match/Vision/PlayerFilter.jsx

+    });
+  }
+
+  generateFilterKey(type) {


don't you need to bind this in the constructor so you don't lose 'this' context?

~~This could be static.~~

Doesn't seems so.

mike-robertson · 2016-11-12T16:11:47Z

src/components/Match/VisionPage.jsx

+    {props.ticks.map((tick) => {
+      const [t, min, max] = [tick, props.min, props.max];
+      const percent = 100 * ((t - min) / (max - min));
+      const cls = [styles['slider-tick']];


use camelCase to avoid this

mike-robertson · 2016-11-12T16:52:28Z

src/components/Match/VisionPage.jsx

+);
+
+const PipelineFilter = (filters, data, iter = Array.prototype.filter) => {
+  const filtered = filters.map(f => iter.call(data, f))


const filteredData = filters.reduce( (arr, filt) => arr.filter(filt), data ); return data.filter(obj => !filteredData.includes(obj));

Doesn't this do the same thing but is easier to understand? Plus no lodash, and you don't need to have a reference to the Array.prototype.filter function.

I'm not 100% sure the implementation is does exactly the same thing.

I can remove the prototype reference.

mike-robertson · 2016-11-12T17:01:26Z

src/components/Match/VisionPage.jsx

+    this.ticks = this.computeTick();
+    this.findPivot = value => _.flow(_.map(x => x.entered.time),
+                                     _.sortedIndex(value))(this.state.wardsLog);
+    this.handleViewportChange = _.debounce(50, this.viewportChange);


why not just do:

this.handleViewportChange = () => setTimeout(this.viewportChange, 50);

No reason to drag in a third party library for that, right?

This is not debouncing, this is deferring. You'll still call on every change event, 50ms after the call stack has been emptied.

mike-robertson · 2016-11-12T17:17:02Z

src/components/Match/renderMatch.js

+
+    const observers = extractWardLog('observer', player.obs_log, player.obs_left_log);
+    const sentries = extractWardLog('sentry', player.sen_log, player.sen_left_log);
+    return _.concat(observers, sentries);


observers.concat(sentries)

mike-robertson · 2016-11-12T17:25:01Z

src/components/Match/renderMatch.js

+    _.flatten,
+    _.sortBy(xs => xs.entered.time),
+    imap((x, i) => ({ ...x, key: i })),
+  );


If this can avoid using lodash, I would prefer that, but I don't know exactly what 'convert' is doing here. It seems weird to me that we would need to flatten this array one level, but I don't know what the data looks like.

The fp flavor cap the iteratee to a certain arrity, this removes that cap. It is basically to have the index in the iteratee function.

mike-robertson · 2016-11-12T17:26:18Z

src/components/Match/renderMatch.js

-          newPlayer.objective_damage[identifier] + player.damage[key] :
-          player.damage[key];
+                                                 newPlayer.objective_damage[identifier] + player.damage[key] :
+                                                 player.damage[key];


not sure I am a fan of this tabbing, but if others are I won't fight it.

mike-robertson · 2016-11-12T17:29:12Z

src/utility/index.jsx

  return input;
 }
+
+export const threshold = _.curry((start, limits, values, value) => {


Same comment here, you could just invoke this function like threshold()()()() and manually curry the function definition but it you are against doing that (so you can do threshold(1, 2, 3)(4)), I'll defer to your judgement.

mike-robertson · 2016-11-12T17:30:46Z

src/utility/index.jsx

+
+  const limitsWithStart = limits.slice(0);
+  limitsWithStart.unshift(start);
+  return findLast(values, (v, i) => _.inRange(limitsWithStart[i], limitsWithStart[i + 1], value));


useful method, but I don't know if it is worth bringing in all of lodash for. Though if we can get away with the 4kb gzipped lodash and you want to use these, I'm down with that. I'm mostly against using lodash when we could instead use native implementations at essentially no extra cost.

howardchung · 2016-11-12T22:59:56Z

will this integrate #445 ?

micaelbergeron · 2016-11-13T13:40:27Z

Let's merge this first then iterate on it if you guys don't mind. #445 is easy to add after.

micaelbergeron · 2016-11-13T13:42:03Z

I didn't remove lodash usage but I tried to use native methods when available and not changing the implementation.

howardchung · 2016-11-14T00:45:27Z

src/components/Match/renderMatch.js

+       })
+    ;
+
+    const observers = extractWardLog('observer', player.obs_log, player.obs_left_log);


will this crash if !player.obs_log (unparsed match?)

I fixed this, I really thought this was handled up in the Component hierarchy.

Match vision tab overhaul

micael.bergeron added 7 commits October 27, 2016 14:15

add ward log draft

bd7e9ae

fix a performance issue for the Vision page

9d1f65f

remove the usage of redux's Connect from the vision page by using the PlainTable instead of the default Table (TableWithOptions). there are still performance issues, buut it is usable.

improve the ward log table

c23f12d

i've decided to not use a <table> anymore to host this data as it seems the material-ui table causes some issues with transitions and animating table row is hacky at best a simple <ul> implementation seems to be more performant

improve the ward log visual

e38c463

polish the wardlog visual

4910708

add the player filter prototypes

4db566a

fix the responsive bugs in the vision page

9e346df

micaelbergeron added type: enhancement type: feature work in progress labels Nov 11, 2016

micaelbergeron self-assigned this Nov 11, 2016

howardchung reviewed Nov 11, 2016

View reviewed changes

howardchung requested changes Nov 11, 2016

View reviewed changes

micaelbergeron mentioned this pull request Nov 11, 2016

Improve ward map visuals #445

Closed

micael.bergeron added 3 commits November 11, 2016 11:10

fixes the errors caused by merging upstream

9916375

Merge remote-tracking branch 'upstream/master' into own_table

843a0c2

# Conflicts: # src/components/Match/VisionMap.jsx # src/utility/index.jsx

apply fixes following code review

3ddc16b

micael.bergeron added 2 commits November 11, 2016 22:03

remove some lint

b7ce459

remove a lint bypass

b4db5a3

micaelbergeron mentioned this pull request Nov 12, 2016

Remove inline eslint rules for globals #476

Closed

micael.bergeron added 2 commits November 11, 2016 22:40

remove a lint bypass

98a2831

refactored Fixed to its own file

890e51f

albertcui approved these changes Nov 12, 2016

View reviewed changes

blukai reviewed Nov 12, 2016

View reviewed changes

fix issues related to the CR

fdf6900

micaelbergeron mentioned this pull request Nov 12, 2016

make player wardmap responsive #390

Closed

micael.bergeron added 4 commits November 12, 2016 08:26

eslint: allow same precedence operators to be mix

12d6828

fixed a problem with a mutation of immutable data

6eb4a48

Revert "eslint: allow same precedence operators to be mix"

60f06bf

This reverts commit 12d6828.

fix linting issues

e827ba7

blukai approved these changes Nov 12, 2016

View reviewed changes

mike-robertson requested changes Nov 12, 2016

View reviewed changes

fix some issues after review

cb0106a

mike-robertson approved these changes Nov 13, 2016

View reviewed changes

howardchung reviewed Nov 14, 2016

View reviewed changes

howardchung approved these changes Nov 14, 2016

View reviewed changes

micael.bergeron added 2 commits November 13, 2016 21:50

fixed a crash when opening an unparsed match

5d8e8f5

coerce the player instead of using a guard

8b2de47

howardchung merged commit 4443323 into odota:master Nov 14, 2016

masad-frost pushed a commit to masad-frost/ui that referenced this pull request Jan 20, 2017

Merge pull request odota#469 from micaelbergeron/own_table

06c563c

Match vision tab overhaul

		@@ -0,0 +1,16 @@
		import React from 'react';

		export const Fixed = (Component) => class extends React.Component {

Conversation

micaelbergeron commented Nov 11, 2016 • edited by howardchung Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micaelbergeron commented Nov 11, 2016

Uh oh!

micaelbergeron commented Nov 12, 2016

Uh oh!

howardchung commented Nov 12, 2016

Uh oh!

blukai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blukai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micaelbergeron Nov 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

micaelbergeron commented Nov 11, 2016 •

edited by howardchung

Loading

micaelbergeron Nov 12, 2016 •

edited

Loading

micaelbergeron Nov 12, 2016 •

edited

Loading