-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix: Navigation between list and isolated mode issues #676
Conversation
Codecov Report
|
Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I'd introduce a constant with all available display modes and import from it instead of using strings.
@@ -6,6 +6,12 @@ import Sections from '../Sections'; | |||
import Section from './Section'; | |||
import { SectionRenderer } from './SectionRenderer'; | |||
|
|||
const options = { | |||
context: { | |||
displayMode: 'all', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd create a enum (Object.freeze
) with all available modes and import it any time you reference it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an example somewhere?
@@ -31,5 +31,5 @@ Section.propTypes = { | |||
}; | |||
|
|||
Section.contextTypes = { | |||
isolatedSection: PropTypes.bool, | |||
displayMode: PropTypes.string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of saying string
it's better to use oneOf
propType with exhausting set of values.
Yeah, good points. I decided to work on that more and was able isolate, fix and test mutation instead of using a hack with By the way how would you ensure that you’re not mutating source objects in tests? |
Not sure about your questions. My points are more about the maintainability. What do you mean by mutating in test? Example? |
I mean tests to prevent mutation in code. That was exactly the issue here. |
Ah no, I don't know how to tests side-effects. |
1. Extract each function to a separate file. 2. Add globalizeComponents function, so getRouteData is now pure. 3. Replace Object.assign with spread operator. 4. Use deepfreeze in tests to prevent mutations.
I’ve added deepfreeze to tests, so it should catch mutations. And I’ve removed cloneDeep hack. I still need to replace strings with an enum. |
Merged manually. |
example, not all of them.
you’ll see isolated mode button below this example instead of a show
all components button.
Browser back / forward buttons also seems to work.
Closes #663, #467