clean structure, resulting in code reduction #10

OriLev · 2018-02-03T15:22:06Z

so here goes for the first narrow scope branch :)

Unite all app headers under <PageHeader> and all main content under <PageMain>, regardless of the current route.

resulted in removal of <GameHeader> and <GameScreen> components. yay less code!

elektronik2k5 · 2018-02-04T16:36:14Z

app/components/Game/Game.js

  };
-  const gameScreenState = {
+  const instructionsProps = { instructionsState, };
+  const instructions = (


If the value is a react component, ItShouldBeCapitalized

Ok, so here I'm a little confused.

if i'd put the wrapper <div> inside the <GameInstructions> then this line would be:

const Instructions = <GameInstructions { ...instructionsProps } >

with the possibility of changing it to const GameInstructions = ... although I doubt I could do it because I already import GameInstructions.

but, the value of putting the jsx structure inside a single word variable is that it will be much more comfortable for use later on.

Although, I could also go for:
const gameHeaderChildren = [ <GameInstructions { ...props } />, <Link { ...otherProps } /> ]

your comment from below about the fine line is very strongly felt..

in the meantime changed to Instructions

elektronik2k5 · 2018-02-04T16:37:27Z

app/components/Game/Game.js

+    </div>
+  );
+  const settingsText = '<-- Settings';
+  const linkProps = {


settingsLinkProps, as these are the props of a very specific link

elektronik2k5 · 2018-02-04T16:37:38Z

app/components/Game/Game.js

+  const linkProps = {
+    className: 'settingsLink',
+    to: { pathname: '/settings', },
+    children: settingsText,


I'd inline it

you mean children: '<---settings'?

done

(also moved between <Link></Link> )

elektronik2k5 · 2018-02-04T16:38:15Z

app/components/Game/Game.js

+    children: settingsText,
+    key: 1,
+  };
+  const settingsLink = <Link { ...linkProps } />;


elektronik2k5 · 2018-02-04T16:41:06Z

app/components/Game/Game.js

+    key: 1,
+  };
+  const settingsLink = <Link { ...linkProps } />;
+  const gameHeaderProps = { children: [ settingsLink, instructions ], };


An array of children requires the use of key props. Don't you get any warnings from the linter and browser console for this?
Also, when the contents of the children are static (like here), instead of using an array with key, you can use the new Fragment syntax, assuming you're using the latest version of React.

Another point here: using a children array with only 2 members decreases readability, since when you're looking at the returned tree from your component, it seems flat - when in fact it isn't. Now, if you have to render a long list or something dynamic, then fine. But here, where everything is static it's worse.
Same goes for pageMainProps below.

It is good that you're learning the various ways to compose components :). It's a fine line knowing which exact tool to use when.

I did add the key prop to both props definitions. maybe that's the reason neither of them complain...
Although the Fragment syntax indeed should be explored.
(I feel like a repeating record with the static/dynamic questions, but what does that mean in this case? The text of the instructions is going to change, but apparently it's not considered dynamic :) please help me to understand this conundrum)

about the second comment, totally agree. feels I overreached in this case with the will to use the children prop. in general I start feeling more and more that having 'smart' components with bigger JSX structure is not that bad if they are kept readable. Surely in this case it will be more readable if all the children will be inside the JSX rather than in the children.

Dynamic in the current context: children, not each child.

If you know ahead of time how many children there are (which has nothing to do with each child), then that's static.
Look at the code: it is an array literal with two values. It can't be anything else, cause it's a literal, like this one [1, 2]. :)
Same goes for an array literal of objects: [a, b] - there are two members: a and b. It doesn't matter what they are, as long as we're talking about the array itself. - It is static.

Dynamic is when you don't know the contents ahead of time, because it's an argument or comes from the user or from some API or whatever. In that case you also can't use a literal to describe it - because you don't have it when you write the code. It only exists at runtime.

Does that help?

yes. very much. thanks!

about "Fragment"

Since I wrote all the children directly in the JSX it seems less relevant now

elektronik2k5 · 2018-02-04T16:46:06Z

app/components/Game/Game.js

+      <Board { ...boardProps } />
+    </div>
+  );
+  const buttonsPanel = <GameButtonsPanel { ...buttonPanelProps } />;


done

although it will move to the JSX as a whole soon enough..

elektronik2k5 · 2018-02-04T16:50:31Z

app/components/PageHeader/PageHeader.js


-export default function PageHeader({ instructions, }) {
-  const instructionsProps = { instructions, };
+export default function PageHeader({ children, }) {


A note on default exports: many people feel strongly about avoiding using them. I didn't for a long time, but have mostly changed my mind and no longer use them. You can google and read why people dislike them.

read about it and it does make sense not to use them, both because of the autocomplete option in IDE's and to sustain a single naming convention for of the imported objects (unless import { foo, } as bar... is used, but that's a whole different topic).

now, how would you suggest introducing this refactor into the project?

On one of the current branches?

On the base/master branch?

Creating a separate branch for this specific piece of refactoring?

elektronik2k5 · 2018-02-04T16:51:51Z

app/components/PageMain/PageMain.js

-export default function PageMain({ setBoardColor, }) {
-  const panelProps = { setBoardColor, };
+
+export default function PageMain({ children, }) {


MainPage is better English

Although I do agree about the sound of it, I wanted to create a separation of a page's header and a page's main section. Which in this case does not necessarily reflect properly in the phrasing 'main page'.

Perhaps I should rename both to PurposeSection..?

FooSection sounds good. I usually set a naming convention, where each file which is a subset of one of the main entity types has it as a suffix in the name. So given these base entities: "Page", "Route", "View", "Widget", "Model", "Store", files would be named "HomePage", "RegistrationRoute", "AddCommentView", "BuySellWidget", "UserModel", "OrdersStore".
It is up to you to define which entities your app has and decide on a naming convention which would make it easier for you to reason about, navigate and make changes in your app.

elektronik2k5 · 2018-02-04T16:54:03Z

app/components/SettingsScreen/SettingsScreen.js

+  const settingsPanelProps = { setBoardColor, key: 1, };
  const mainProps = {
-    setBoardColor,
+    children: [ <SettingsPanel { ...settingsPanelProps } /> ],


Same here about children array of static members

elektronik2k5

You commented that you did changes, but I don't see them here in this PR.

clean structure, resulting in code reduction

5d096c3

OriLev self-assigned this Feb 3, 2018

OriLev requested a review from elektronik2k5 February 3, 2018 15:22

OriLev assigned elektronik2k5 Feb 3, 2018

elektronik2k5 requested changes Feb 4, 2018

View reviewed changes

elektronik2k5 reviewed Feb 9, 2018

View reviewed changes

OriLev added 3 commits February 10, 2018 17:26

changed the naming to HeaderSection and MainSection

4c0f6f3

fix PropTypes to accept children as elements and as an array

2485340

refactor the presentation of HeaderSection and MainSection children

4c050f0

clean structure, resulting in code reduction #10

Are you sure you want to change the base?

clean structure, resulting in code reduction #10

Uh oh!

Conversation

OriLev commented Feb 3, 2018

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!

elektronik2k5 Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

elektronik2k5 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elektronik2k5 Feb 9, 2018 •

edited

Loading