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

[WIP] refactor(display-name): rendered the component in react instead of backbone #1571

Closed
wants to merge 4 commits into from

Conversation

hritvi
Copy link
Contributor

@hritvi hritvi commented Jun 12, 2019

Worked on rendering the display-name component in react.
Still, need to work on these things:

  • Writing tests
  • Localize the component[The component in only being rendered in English, translator still needs to be added]

Some screenshots:
Screenshot from 2019-06-12 10-16-02

Screenshot from 2019-06-12 10-16-18

Screenshot from 2019-06-12 10-16-53

Screenshot from 2019-06-12 10-17-15

Copy link
Contributor

@lmorchard lmorchard left a comment

Choose a reason for hiding this comment

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

Looking pretty good so far! Added a few comments about considerations for testing.

I'd also suggest looking into using react-testing-library because it has good utilities for rendering React components and then inspecting them for expected results.

Over in the fxa-payments-server package, I've had a pretty good experience using react-testing-library to write tests like these for a Tooltip component.

There may be a challenge or two in getting React and JSX tests running in fxa-content-server with Mocha. That's part of why I suggested writing them separately from other existing Backbone tests. But, we'll probably all learn something from the effort 😅

Feel free to ping me on Slack with any questions or if you need help!

presets: ['@babel/preset-env'],
presets: [
[
'@babel/preset-react', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this was easier to get included in the build process than I feared. Hooray!


const t = msg => translator.get(msg);

function DisplayName(props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding export to all these React components. While we do export the View at the end of the module, being able to access each individual component separately might make it easier to test things in smaller pieces.

</header>
);
}
function ChangeorAddButtonComponent(props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For example, if this component were exported, you could write a small test for ChangeorAddButtonComponent in isolation that exercises what it does with and without displayName defined.

You might also be able to separate React tests from the rest of the existing tests. I have a suspicion that might make things simpler.

@hritvi
Copy link
Contributor Author

hritvi commented Jul 1, 2019

Have opened a new PR for it: #1611. Closing this.

@hritvi hritvi closed this Jul 1, 2019
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