Scaffold playground page #8

ning-y · 2018-05-16T04:18:41Z

Addresses #2, 'Scaffold Playground Layout'.

Playground is a child of application
Playground is associated with new state IPlaygroundState which is a child of IState
IPlaygroundState stores the contents of the playground editor, allowing it to persist between pages
Use the library react-ace to implement the editor

No functionality.

Oops

Changes: - Changed `Playground` component to a statefull one (extends `React.Component) - add property `playgroundCode` to IApplicationState (bad practice, change to a slice of the state instead) - add interface `IPlaygroundProps` to denote the properties passed to Playground

Once again, this is a WIP. Text typed into the playground editor will stay in the playground (or rather, appear again) after leaving and coming back. Changes: - added of action, actionCreator (updatePlaygroundCode) - added a reducer under application's `reducer` function Possible problems: - Location of reducer - Location of playgroundCode stored (as in previous commit)

Made a compromise by including an IPlaygroundState as part of IApplicationProps

evansb · 2018-05-16T10:45:02Z

src/reducers/playground.ts

+import { IUpdateEditorValue, UPDATE_EDITOR_VALUE } from '../actions/playground'
+
+/**
+ * A state for the playground container


In general, since we have established a convention, I think the comment can be terser and only to describe non-obvious name or to explain a complex routine/function.

I think we can go with no comment by default, the reviewer will be the one responsible for requesting documentation if deemed necessary, what do you think?

I'm ok with removing it for now, and add it later on if necessary. For now, the properties and name do seem to be sufficiently self-explanatory.

evansb · 2018-05-16T10:46:25Z

src/actions/playground.ts

+ * @property newEditorValue - The new string value for `editorValue`
+ */
+export interface IUpdateEditorValue extends Action {
+  type: string


This should be redundant, all Action is expected to have a type field, please verify

evansb · 2018-05-16T10:47:41Z

src/actions/playground.ts

+ */
+export interface IUpdateEditorValue extends Action {
+  type: string
+  newEditorValue: string


You should follow the Redux FSA(Flux Standard Action) convention. The payload of the action should be under the name payload. A payload is typically an object in complex action, but in this case I think a string suffice.

To clarify, do you mean the naming convention only? Or do you mean that we should keep thepayload object as an arbitrarily typed object (like in most example online)? I feel that the typing should remain strict for a particular payload.

evansb · 2018-05-16T10:49:27Z

src/components/Application.tsx

 }

-const Application: React.SFC<IApplicationProps> = ({ application }) => {
+const Application: React.SFC<IApplicationProps> = (props: IApplicationProps) => {


Adding IApplicationProps after props: is redundant because we already declare the type as React.SFC<IApplicationProps>. TypeScript is able to infer the parameter type. So you can unpack { title } straight away in the parameter.

I feel like it would be better to keep a props argument as compared to unpacking it to { title }, so that adding properties to IApplicationProps does not require an additional change in the arguments. What do you think?

evansb · 2018-05-16T10:49:44Z

src/actions/playground.ts

+ * The `type` attribute for an `Action` which updates the `IPlaygroundState`
+ * `editorValue`
+ */
+export const UPDATE_EDITOR_VALUE: string = 'UPDATE_EDITOR_VALUE'


string is redundant

When in doubt, you can check with your editor /IDE, if upon hovering to the variable you see a type, that means the type can be inferred.

Of course sometimes you want to explicitly declare it anyway, for documentation purposes

evansb · 2018-05-16T10:53:58Z

src/components/Playground.tsx

+ */
+export interface IPlaygroundProps {
+  editorValue: string
+  updateCode: (newCode: string) => void


Usually the convention for event handler begins with handle<event-name>, i.e handleEditorChange

evansb · 2018-05-16T10:54:13Z

src/components/Playground.tsx

+/**
+ * A component representing the Playground
+ */
+export default class Playground extends React.Component<IPlaygroundProps, {}> {


You should always default to React.SFC until you need state

In this case, our editor would need to call upon a dispatch to update the editorValue within the IPlaygroundState, whenever someone edits code in the ace editor. Is a Component warranted in this case?

Yeah, so in this case the editor component is stateful, but the playground component itself should be stateless

Ah, I understand now. Will do!

Changes not made: - Change of `Playground` component to an `SFC`

Due to 7ac707c.

This is done by abstracting out the ace editor into its own component , `Editor`, and its own container to handle redux calls, `EditorContainer`.

Export of `Editor` changed to a default export, like the other components. A test has been added for the `Editor` component. Snapshot changes pending.

Mission edit assess edit

ning-y and others added 26 commits May 14, 2018 10:48

Add button for playground at Navbar

de9df9b

No functionality.

Make first NavbarGroup's align left explicit

42d4cb6

Change playground icon

e917cf0

Add routes to Playground

c00bfeb

Update test snapshots

c70d46a

Fix bad test for Playground component

0ab401a

Oops

Add react-ace for IDE

c9de874

Try passing props into playground

b4f37b7

Remove unused onChange()

8145a2a

Change initialValue prop to editorValue

48bd369

Made playground actions a separate file

023f277

Modify actions and dispatch call

dd585f9

Change redux import statement

053b2c9

Add IPlaygroundState to IState in index reducer

b17cb25

Add playground reducer

8d480a3

Remove playgroundCode from IApplicationState

7c3ad6d

Fix type errors

fcf1c60

Made a compromise by including an IPlaygroundState as part of IApplicationProps

Re-disable console.logs (tslint)

4004d64

Fix missing imports for react-ace mode, theme

1879083

Add playground CSS

1f5edb7

Add JSDoc style comments for playground

376ec46

Fix some tests

eb37db7

Fix semantics of mapStateToProps for ApplicationContainer

931a204

Do trivial formatting

b451e08

remo5000 added the Enhancement New feature request label May 16, 2018

remo5000 added this to the Sprint 1 milestone May 16, 2018

remo5000 requested a review from evansb May 16, 2018 04:25

evansb requested changes May 16, 2018

View reviewed changes

remo5000 and others added 6 commits May 16, 2018 23:38

Make 16/05 code review changes

7ac707c

Changes not made: - Change of `Playground` component to an `SFC`

Fix test snapshot (name of onChange for ReactAce)

b2a3d60

Due to 7ac707c.

Revert Playground as SFC, make Editor container

caf551b

Make Playground stateless

51ea83d

This is done by abstracting out the ace editor into its own component , `Editor`, and its own container to handle redux calls, `EditorContainer`.

Add test and fix export type

9758bb7

Export of `Editor` changed to a default export, like the other components. A test has been added for the `Editor` component. Snapshot changes pending.

Fix test snapshots

a203e5e

evansb approved these changes May 18, 2018

View reviewed changes

evansb merged commit e7a64b9 into master May 18, 2018

ning-y deleted the scaffold-playground branch May 18, 2018 10:00

gerhean added a commit that referenced this pull request Mar 14, 2019

Merge pull request #8 from gerhean/mission-edit-assess-edit

e52ca30

Mission edit assess edit

renovate bot mentioned this pull request Jun 6, 2023

fix(deps): update all non-major dependencies #2192

Closed

1 task

Scaffold playground page #8

Scaffold playground page #8

Uh oh!

Conversation

ning-y commented May 16, 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!

remo5000 May 16, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

remo5000 May 16, 2018 •

edited

Loading