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

Fixtures #103

Merged
merged 41 commits into from Mar 6, 2020
Merged

Fixtures #103

merged 41 commits into from Mar 6, 2020

Conversation

@andyrichardson
Copy link
Contributor

andyrichardson commented Mar 3, 2020

About

Adds fixtures!

Fix #102

Changes

  • Add fixtures (obviously)
  • Replace prop drilling w/ context
  • Replace "update flash" logic with React Spring
  • Clean up folder structures
  • Add visual regression testing
  • Speed up dependency installation in CI

Screenshots

Kapture 2020-03-03 at 19 11 29

Copy link
Member

sofiapoh left a comment

Looks good! This is a very welcome change and improvement for developer experience.

I think it might be nicer to look at the fixtures if we could move some of the mock data to a separate file/bottom of the file, just a lot of visual clutter before we get to the actual test. What do you think?

I also noticed you removed HighlightUpdate from the ListItem, was there a specific reason for this change?

cosmos.override.js Outdated Show resolved Hide resolved
@andyrichardson

This comment has been minimized.

Copy link
Contributor Author

andyrichardson commented Mar 4, 2020

@sofiapoh yeah I'm down for colocating data if you have suggestions on how we could do that!

I found that most fixtures require very specific mocked states (usually unrelated to other fixtures) so I'm not sure how/if we would should merge them into one. I've gone as far as to not share any values between fixtures because deeply nested spreading hasn't been fun and its felt easier to just copy-paste/simulate real world values - but maybe there's a better way to do this!

I also noticed you removed HighlightUpdate from the ListItem, was there a specific reason for this change?

It's back! Disabled it while trying to hook up context 👍

interface Props {
children: ReactNode;
export interface ExplorerContextValue {
operations: NodeMap;

This comment has been minimized.

Copy link
@andyrichardson

andyrichardson Mar 4, 2020

Author Contributor

Moved explorer-wide values to context so we don't need to pass them down

const [focusedNodeId, setFocusedNodeId] = useState<string | undefined>(
undefined
);
const [detailViewNode, setDetailViewNode] = useState<FieldNode | null>(null);

This comment has been minimized.

Copy link
@andyrichardson

andyrichardson Mar 4, 2020

Author Contributor

This is now in context (focusedNode)


export function Explorer() {
const operations = useContext(ExplorerContext);
const [focusedNodeId, setFocusedNodeId] = useState<string | undefined>(

This comment has been minimized.

Copy link
@andyrichardson

andyrichardson Mar 4, 2020

Author Contributor

I coudn't work out how this is any different to detailViewNode and wanted to trim down state + renders - from what I can tell, both values get set at the same time

@@ -49,10 +36,6 @@ const ListContainer = styled.section`
padding-right: 1rem;
padding-bottom: 1rem;
overflow: auto;
@media (min-aspect-ratio: 1/1) {

This comment has been minimized.

Copy link
@andyrichardson

andyrichardson Mar 4, 2020

Author Contributor

This was probably needed before we updated the Pane component

const props = { value, expand };

if (value instanceof Array) {
return <ArrayValue {...props} value={value as any[]} />;

This comment has been minimized.

Copy link
@andyrichardson

andyrichardson Mar 4, 2020

Author Contributor

I've broken these conditions out and we have each different state as a fixture now

color: ${p => p.theme.grey["+2"]};
background: ${props => props.theme.dark["-2"]} !important;
color: #fff;
background: ${props => props.theme.blue["0"]};

This comment has been minimized.

Copy link
@andyrichardson

andyrichardson Mar 4, 2020

Author Contributor

Fixed background color not changing - also fixtured

@andyrichardson andyrichardson force-pushed the fixtures branch 2 times, most recently from 6d412d2 to a507784 Mar 5, 2020
@andyrichardson andyrichardson force-pushed the fixtures branch from b66c497 to f7ad9e6 Mar 5, 2020
@andyrichardson andyrichardson force-pushed the fixtures branch from f7ad9e6 to 3d167ca Mar 5, 2020
@andyrichardson andyrichardson force-pushed the fixtures branch from 0a31994 to d5b30ac Mar 6, 2020
andyrichardson added 12 commits Mar 6, 2020
@andyrichardson andyrichardson force-pushed the fixtures branch 4 times, most recently from 82d3014 to 24e7957 Mar 6, 2020
@andyrichardson andyrichardson force-pushed the fixtures branch from 24e7957 to 74e3b78 Mar 6, 2020
@andyrichardson andyrichardson merged commit 6dc46c8 into master Mar 6, 2020
7 checks passed
7 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint firefox Your tests passed on CircleCI!
Details
ci/circleci: prettier Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: visual regression Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.