Heatmap #9

estuelke · 2020-06-14T15:34:15Z

Got all the code in place and working but having problems implementing the tests. I keep getting:

Warning: An update to Search inside a test was not wrapped in act(...).
    
    When testing, code that causes React state updates should be wrapped into act(...):
    
    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */

I've tried wrapping everything from just the render to both the render and fetch mocks. Nothing works. However, I did stop getting the message if I only called the render method without const { getByRole }. When I did some console prints, I noticed that the heatmap isn't getting written into the test. I am guessing that's why the tests are throwing that warning message. But I am not sure how to fix it.

…ues for now

jkettmann · 2020-06-14T19:06:13Z

src/tests/Heatmap.test.js

+    const { waitForNextUpdate } = renderHook(() => useFetchPosts('kittens'));
+
+    await waitForNextUpdate();
+    await act(async () => {


I guess there are a couple of smaller problems.

For the test of the search page these three lines are not necessary. They are only required for testing a custom hook, but not when you want to test a component or page

Maybe you did that already: You can use the debug function that is returned from render to see the DOM. So when you have a test like this you will see no heatmap table as you mentioned.

fetch.once... const { getByRole, debug } = renderSearchPage('/search/kittens'); debug()

That's because the (mock) fetching is not done yet. During the initial render the heatmap is not visible.
3. Instead of the getByRole query you can use findByRole. This one is async, so you have to await it. When you run the test now you should see the act warnings plus a different error: MutationObserver is not a constructor
4. This leads us to this comment where you can find the solution. When you add debug() now after findByRole you should see the table in the output

That worked! Thanks

Oddly, the tests are passing locally, but not remotely even though they should be using the same data, right? Am I missing something?

Perfect, glad that it worked 👍

The problem with the failing tests in the CI is probably that the GitHub pipelines have a different timezone than your local machine. You need to set a fixed timezone. You can check the cypress scripts inside package.json. There's an environment variable TZ that you can use

jkettmann

Looks very nice 👍 Of course, I left some comments 😄

package.json

src/components/Heatmap.js

jkettmann · 2020-06-15T20:00:06Z

src/components/HeatmapCell.js

+  const [clicked, setClicked] = useState(false);
+
+  const handleClick = () => {
+    setClicked(true);
+    handleCellClick(id);
+  };
+
+  useEffect(() => {
+    if (clickedCellId === id) {
+      setClicked(true);
+    } else {
+      setClicked(false);
+    }
+  }, [clickedCellId, id]);


This is a very common mistake and you might be surprised how much simpler it can be 😄

clicked is just derived from the clickedCellId and id props. So you can simply replace the state variable by const clicked = clickedCellId === id;as you did in theuseEffect`. Try it out, your component will look much simpler

woah! So much better. Thanks! I was overthinking it, it seems

jkettmann · 2020-06-15T20:00:30Z

src/styles/themes.js

@@ -39,12 +39,30 @@ const mainTheme = {
    externalLink: '#0087ff',
    searchBoxBorder: '#e6e6e6',
    spinner: '#fdb755',
+    tablecolumnheader: '#787878',


I'd probably use camelCase here 😉

oops... my python is showing 😅

jkettmann · 2020-06-15T20:00:40Z

src/styles/themes.js

+    count: {
+      0: '#e0e592',
+      1: '#aed396',
+      2: '#a9d194',
+      3: '#a0ce93',
+      4: '#99cd94',
+      5: '#8cc894',
+      6: '#5eb391',
+      7: '#5db492',
+      8: '#5cb391',
+      9: '#5aad8c',
+      10: '#3984a3',
+    },


Perfect 🚀

jkettmann · 2020-06-15T20:01:34Z

src/tests/Heatmap.test.js

+    const originalStyle = window.getComputedStyle(tableCells[1]);
+    const originalBorder = originalStyle.getPropertyValue('border');
+    expect(originalBorder).toEqual('1px solid #a0ce93');
+
+    fireEvent.click(tableCells[1]);
+
+    const updatedStyle = window.getComputedStyle(tableCells[1]);
+    const updatedBorder = updatedStyle.getPropertyValue('border');
+    expect(updatedBorder).toEqual('1px solid #1e2537');


Haven't seen anyone testing the borders yet! 🎉

jkettmann · 2020-06-15T20:02:52Z

src/tests/Heatmap.test.js

+
+  it('cell highlights on click', async () => {
+    let tableCells;
+    await act(async () => {


This is not required. You can remove the act

jkettmann

Almost good to go. I just left a few more comments for smaller optimizations

jkettmann · 2020-06-21T06:47:42Z

src/components/Heatmap.js

+  '12:00pm', '2:00pm', '4:00pm', '6:00pm', '8:00pm', '10:00pm',
+];
+
+const getCounts = (posts) => {


This name is very generic. Can you find a more descriptive name? Remember the email lesson about variable/function naming and the 5 Whats.

This also goes for the counts array below

jkettmann · 2020-06-21T06:48:17Z

src/components/Heatmap.js

+`;
+
+const Timezone = styled.span`
+    font-weight: bold;


indentation

jkettmann · 2020-06-21T06:49:13Z

src/components/Heatmap.js

+            return (
+              <tr key={dayOfWeek}>
+                <RowHeader key={dayOfWeek}>{dayOfWeek}</RowHeader>
+                {days.map((hourCount, hour) => {


indentation

I think this is okay? Since days.map... isn't part of RowHeader? I separated out RowHeader into three lines to make it clearer.

jkettmann · 2020-06-21T06:49:58Z

src/components/Heatmap.js

@@ -1,5 +1,129 @@
-import React from 'react';
+import React, { useState } from 'react';


This file looks very simple compared to other solutions I've seen. Good job 👍

jkettmann · 2020-06-21T06:55:50Z

src/components/Heatmap.js

+          {counts.map((days, day) => {
+            const dayOfWeek = daysOfWeek[day];
+            return (
+              <tr key={dayOfWeek}>


You could extract this into its own component. This would potentially allow you to apply performance optimizations with React.memo.

That's a bit premature optimization though. So no problem if you keep it like it is. I just wanted you to know about React.memo 😄

jkettmann · 2020-06-21T06:58:29Z

src/components/Heatmap.js

+`;
+
+const Heatmap = ({ posts }) => {
+  const counts = getCounts(posts);


This is run whenever the Heatmap is re-rendered (e.g. when a cell is clicked). Since the function iterates over 500 elements you might want to optimize it performance-wise.

By using useMemo you can call this function only when posts changes.

jkettmann · 2020-06-21T07:01:29Z

src/components/HeatmapCell.js

+  font-size: ${({ theme }) => theme.font.size.small};
+  background-color: ${({ theme, colorValue }) => theme.color.count[colorValue]};
+  border: 1px solid ${({ theme, colorValue, clicked }) => (
+    clicked ? theme.color.tableRowHeader : theme.color.count[colorValue]


Nice usage of styled-components props 👍

jkettmann · 2020-06-21T07:05:31Z

src/components/HeatmapCell.js

+  }
+`;
+
+const HeatmapCell = ({


Just a small suggestion 😄 You could make this component shorter by removing the const.

const HeatmapCell = ({ children, colorValue, handleCellClick, id, clickedCellId, }) => ( <TableCell colorValue={colorValue} clicked={clickedCellId === id} onClick={() => handleCellClick(id)} > {children} </TableCell> );

I love these kinds of suggestions. I think I get caught up in the tiny details sometimes and I forget to step back and see where I can make things more efficient.

…plified some code

jkettmann

Looks great! 😄

jkettmann · 2020-06-21T19:07:31Z

Btw change the title 😉 I recently discovered that you can actually create a draft PR instead of WIP prefix.

Erin Stuelke added 3 commits June 13, 2020 21:07

implemented heatmap

cd9df61

added back posts in post table for placeholder to prevent linting iss…

48af76b

…ues for now

began implementing tests but having problems with wrapped in act warning

fe2c464

jkettmann reviewed Jun 14, 2020

View reviewed changes

Erin Stuelke added 2 commits June 14, 2020 16:58

implemented tests

a3711c8

udpated jest to run with America/New_York timezon

b024ecb

estuelke requested a review from jkettmann June 15, 2020 14:05

jkettmann requested changes Jun 15, 2020

View reviewed changes

Erin Stuelke added 2 commits June 20, 2020 20:29

simplified components, cleaned up unecessary code

eaf1efb

updated tests to remove unecessary act

cf6bddb

estuelke requested a review from jkettmann June 21, 2020 00:38

jkettmann requested changes Jun 21, 2020

View reviewed changes

Erin Stuelke added 2 commits June 21, 2020 11:24

separated components, use memoization for post count calculation, sim…

233baf7

…plified some code

added useMemo dependency

0462705

estuelke requested a review from jkettmann June 21, 2020 15:35

jkettmann approved these changes Jun 21, 2020

View reviewed changes

estuelke merged commit c44f8e9 into master Jun 23, 2020

estuelke deleted the heatmap branch June 23, 2020 22:54

estuelke changed the title ~~[WIP] Heatmap~~ Heatmap Jun 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Heatmap #9

Heatmap #9

estuelke commented Jun 14, 2020

jkettmann Jun 14, 2020

estuelke Jun 14, 2020

estuelke Jun 14, 2020

jkettmann Jun 15, 2020

jkettmann left a comment

jkettmann Jun 15, 2020

estuelke Jun 21, 2020

jkettmann Jun 15, 2020

estuelke Jun 21, 2020

jkettmann Jun 15, 2020

jkettmann Jun 15, 2020

jkettmann Jun 15, 2020

jkettmann left a comment

jkettmann Jun 21, 2020

jkettmann Jun 21, 2020

jkettmann Jun 21, 2020

estuelke Jun 21, 2020

jkettmann Jun 21, 2020

jkettmann Jun 21, 2020

jkettmann Jun 21, 2020

jkettmann Jun 21, 2020

jkettmann Jun 21, 2020

estuelke Jun 21, 2020

jkettmann left a comment

jkettmann commented Jun 21, 2020

		@@ -1,5 +1,129 @@
		import React from 'react';
		import React, { useState } from 'react';

Heatmap #9

Heatmap #9

Conversation

estuelke commented Jun 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkettmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkettmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkettmann left a comment

Choose a reason for hiding this comment

jkettmann commented Jun 21, 2020