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

[Spritelab][Poem Bot HOC] Initial Poem Bank prototype #40830

Merged
merged 2 commits into from
Jun 2, 2021

Conversation

ajpal
Copy link
Contributor

@ajpal ajpal commented May 27, 2021

Behind experiment flag poemBot

Modal to pick a poem from the poem bank
image

Selected poem is displayed under the playspace:
image

For now poem bank is defined in poems.json but my guess is that at some point we may want to build a LB edit page similar to http://levelbuilder-studio.code.org/datasets for managing poems.

The UI is completely non-finalized, the main purpose is to just get a working prototype for Amy BW to be able to write lessons around.

Links

jira

Testing story

Going to hold off on unit tests until we finalize design

Deployment strategy

Follow-up work

  • Make poem bank per-level configurable

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@ajpal ajpal requested a review from a team May 27, 2021 22:29
@@ -57,6 +59,9 @@ class P5LabVisualizationColumn extends React.Component {

constructor(props) {
super(props);
this.spritelabPoemBotExperiment = experiments.isEnabled(
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure how much we care about this, but we could check isSpritelab as part of this conditional to make sure <PoemBank/> is only displayed in spritelab

};

selectPoem() {
this.props.onSelect(this.props.name, this.props.author, this.props.lines);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this line would be more readable if we first destructured onSelect, name, author, and lines from props

selectedPoem: null
};

class PoemTile extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

this component doesn't have any state, so i think we should refactor it to a functional component. react is standardizing on functional over class components, so i'm trying to move us toward functional components wherever we can

Copy link
Contributor

Choose a reason for hiding this comment

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

for functional components, i usually set them up like this:

function MyComponent(props) {
  function otherFunction() {
    // i still have access to `props`
  }

  return <div>{props.text}</div>;
}

MyComponent.propTypes = {
  text: PropTypes.string.isRequired
};

MyComponent.defaultProps = {
  text: 'Hello, world!'
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! that feels very clean for a component as small and simple as this :)

}

export default class PoemBank extends React.Component {
static propTypes = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this line can be removed if we don't have any props for this component

height: '100%'
},
poemTile: {
border: '1px solid black',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: when we finalize this, we should use colors from our color constants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah definitely, this is not at all the finalized design- I just added a black border so I could see the components better :)

@ajpal ajpal merged commit 0463000 into staging Jun 2, 2021
@ajpal ajpal deleted the may27-poem-bank-prototype branch June 2, 2021 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants