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

Clean up on style options vs. props #2310

Closed
compulim opened this issue Aug 13, 2019 · 3 comments
Closed

Clean up on style options vs. props #2310

compulim opened this issue Aug 13, 2019 · 3 comments
Labels
backlog Out of scope for the current iteration but it will be evaluated in a future release. bug Indicates an unexpected problem or an unintended behavior. front-burner p1 Painful if we don't fix, won't block releasing
Milestone

Comments

@compulim
Copy link
Contributor

compulim commented Aug 13, 2019

Version

master

Describe the bug

Our React context (namely, Composer.js) and Redux store is kind of a bin to throw stuff at, we didn't clear up the responsibility of each. Also same for style options vs. props.

### React Context vs. Redux store

Redux store should only hold UI-agnostic content, for example, activities. It should not hold UI features, like language. Redux store is designed to be able to bring in independently to React Native. Or maybe Angular thru NgRx, but not guaranteed.

React store should keep UI features, like language and which activities has been spoken.

Things keep in Redux store can go thru de/hydration loop. Think about survivability across F5. Speech synthesis doesn't meet this bar (not desirable to be survived).

Style options vs. props

Style options is designed to be a flat dictionary and easily serializable as query string. We should keep styles in it, including temporary styles, for example, groupTimestamp and sendTimeout. The only exception is disabled, because it is React convention.

Props keep things unrelated to styling, for example, Redux store, renderer middleware, ponyfills. Usually, they are heavy objects or functions.

What is not great today

This listing is for stuff that are not great.

  • Connect to Direct Line
    • May be this is inevitable, must be part of initialization
  • Send timeout could be part of the style options
    • Arguably, when to show the "retry" prompt is a styling things, not a logic currently
    • Store is not interested in retry and is not aware of retry logic
      • Yes, we don't have a good retry story, we can't retry postBack and messageBack

[Bug]

@compulim compulim added bug Indicates an unexpected problem or an unintended behavior. Pending labels Aug 13, 2019
@compulim compulim changed the title Clean up on React context, Redux store, and style options vs. props Clean up on React context vs. Redux store, and style options vs. props Aug 13, 2019
@corinagum corinagum removed the Pending label Aug 27, 2019
@compulim
Copy link
Contributor Author

compulim commented Sep 18, 2019

Will partly fix by #2364. The part being fixed is Context vs. Composer. Part not fixed is style options vs. props.

@corinagum corinagum added backlog Out of scope for the current iteration but it will be evaluated in a future release. and removed backlog Out of scope for the current iteration but it will be evaluated in a future release. labels Sep 23, 2020
@compulim compulim added the backlog Out of scope for the current iteration but it will be evaluated in a future release. label Oct 5, 2020
@corinagum
Copy link
Contributor

Updated to remove Context vs Composer items that have been addressed.

@corinagum corinagum changed the title Clean up on React context vs. Redux store, and style options vs. props Clean up on style options vs. props Oct 27, 2020
@corinagum corinagum added this to the R12 milestone Dec 21, 2020
@corinagum
Copy link
Contributor

Needs prioritization.

@corinagum corinagum added the p1 Painful if we don't fix, won't block releasing label Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Out of scope for the current iteration but it will be evaluated in a future release. bug Indicates an unexpected problem or an unintended behavior. front-burner p1 Painful if we don't fix, won't block releasing
Projects
None yet
Development

No branches or pull requests

2 participants