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

Snackbar for Avatar code updated feedback #799

Merged
merged 14 commits into from
Sep 12, 2018
Merged

Snackbar for Avatar code updated feedback #799

merged 14 commits into from
Sep 12, 2018

Conversation

mrniket
Copy link
Contributor

@mrniket mrniket commented Sep 6, 2018

ezgif com-optimize

Notable changes:

  • Snackbar
  • IDE now uses a dark theme while the rest of the UI uses a light theme
  • createShallowWithTheme has been modified to allow testing of the Snackbar and handle the two themes
  • Socket IO events

This change is Reviewable

Copy link
Contributor

@NiallEgan NiallEgan left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 26 files at r1.
Reviewable status: 8 of 26 files reviewed, all discussions resolved

Copy link
Contributor

@TheseusGrey TheseusGrey left a comment

Choose a reason for hiding this comment

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

Reviewed 18 of 26 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mrniket)


game_frontend/src/testHelpers/createShallow.js, line 7 at r1 (raw file):

const light = ThemeProvider.createTheme('light')
const dark = ThemeProvider.createTheme('dark')
const themeVariants = { light, dark }

Is it needed for you to store the themes to their own variables before adding them to themeVariants?

Copy link
Contributor Author

@mrniket mrniket left a comment

Choose a reason for hiding this comment

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

Reviewable status: 23 of 26 files reviewed, 1 unresolved discussion (waiting on @NiallEgan, @dent50cent, and @mrniket)


game_frontend/src/testHelpers/createShallow.js, line 7 at r1 (raw file):

Previously, dent50cent wrote…

Is it needed for you to store the themes to their own variables before adding them to themeVariants?

It's not needed but it's an optimisation as it will stop the theme being generated for every test that needs one

Copy link
Contributor

@TheseusGrey TheseusGrey left a comment

Choose a reason for hiding this comment

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

Reviewable status: 23 of 26 files reviewed, 1 unresolved discussion (waiting on @NiallEgan and @dent50cent)


game_frontend/src/testHelpers/createShallow.js, line 7 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

It's not needed but it's an optimisation as it will stop the theme being generated for every test that needs one

:OK:

Copy link
Contributor

@TheseusGrey TheseusGrey left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mrniket)

Copy link
Contributor

@riaJha97 riaJha97 left a comment

Choose a reason for hiding this comment

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

Reviewed 21 of 26 files at r1, 2 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mrniket)

…f components

No simple way to compose context
Copy link
Contributor

@TheseusGrey TheseusGrey left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 8 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mrniket)


game_frontend/src/containers/IDE/index.js, line 33 at r3 (raw file):

        </MuiThemeProvider>
      </StyledComponentsThemeProvider>
    )

Do you need to specify the theme for both the MuiThemeProvider and StyledComponentsThemeProvider?

Copy link
Contributor Author

@mrniket mrniket left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mrniket)


game_frontend/src/containers/IDE/index.js, line 33 at r3 (raw file):

Previously, dent50cent wrote…

Do you need to specify the theme for both the MuiThemeProvider and StyledComponentsThemeProvider?

Yeah, the providers are from two different libraries (material-ui and styled-components) and as such are independent of each other

Copy link
Contributor

@riaJha97 riaJha97 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 26 files at r1, 1 of 3 files at r2, 5 of 8 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mrniket and @dent50cent)

Copy link
Contributor

@TheseusGrey TheseusGrey left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mrniket mrniket merged commit d25fe20 into master Sep 12, 2018
@mrniket mrniket deleted the snackbar branch September 12, 2018 11:18
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

4 participants