Skip to content

Adds initial router, routes, and placeholder views#14862

Closed
ryanahamilton wants to merge 4 commits into
apache:masterfrom
astronomer:ui_scaffold_routes
Closed

Adds initial router, routes, and placeholder views#14862
ryanahamilton wants to merge 4 commits into
apache:masterfrom
astronomer:ui_scaffold_routes

Conversation

@ryanahamilton

Copy link
Copy Markdown
Contributor

Resolves #14802.

This adds react-router-dom with an initial collection of Routes and corresponding placeholder view components.

@ryanahamilton ryanahamilton requested a review from ashb as a code owner March 17, 2021 20:30
@boring-cyborg boring-cyborg Bot added the area:UI Related to UI/UX. For Frontend Developers. label Mar 17, 2021
@ryanahamilton ryanahamilton added the AIP-38 Modern Web Application label Mar 17, 2021
@bbovenzi

bbovenzi commented Mar 17, 2021

Copy link
Copy Markdown
Contributor

Looks good! But I think the test will break. Could we update it to test the router (ie pipelines will render on / and the 404 page will show up on /invalid-path?

@ryanahamilton ryanahamilton force-pushed the ui_scaffold_routes branch 2 times, most recently from 39683c7 to 331ebf8 Compare March 18, 2021 02:33
Comment thread airflow/ui/src/App.tsx
</Center>
<Switch>
<Redirect exact path="/" to="/pipelines" />
<Route exact path="/pipelines" component={Pipelines} />

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
<Route exact path="/pipelines" component={Pipelines} />
<Route exact path="/workflows" component={Workflows} />

etc?

Most of the place in Airflow docs we call it DAGs (most commonly) or Workflows less frequently. Pipeline is very rarely used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These are just placeholders, nothing is set in stone. Once we have a full picture of what the new information architecture looks like, we'll be seeking feedback.

Comment thread airflow/ui/src/App.tsx
<Route exact path="/access/users/:id/edit" component={Users} />
<Route exact path="/access/roles" component={Roles} />

<Route exact path="/docs" component={Docs} />

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This was previously just a link out to docs -- do we need/want it to be a component now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. Again, nothing set in stone, but we're thinking we can provide a better experience with a page like we mocked up in the POC.

@bbovenzi

Copy link
Copy Markdown
Contributor
  • Updated the tests to properly test routing.
  • Fixed yarn lint by stopping it from doing typescript checks on node_modules packages
  • Workaround to prevent the motion() warning when running tests

Comment thread airflow/ui/package.json Outdated
"webpack-cli": "^3",
"webpack-dev-server": "^3"
},
"resolutions": {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's this key for? Not seen this one before.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A warning was bubbling up from Chakra UI. They're using a deprecated function from a dependency. resolutions let's us swap out their dependency to a slightly older one which doesn't fire the warning. We can remove it when Chakra releases an update.

See:
chakra-ui/chakra-ui#3459 (comment)
https://classic.yarnpkg.com/en/docs/selective-version-resolutions/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ohh yeah yarn fix for updating nested dependencies. Gotcha.

We can keep this if it's helpful.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yarn was erroring in the CI build so we're seeing if this caused it.

@github-actions github-actions Bot added the full tests needed We need to run full set of tests for this PR to merge label Mar 18, 2021
@github-actions

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@ryanahamilton ryanahamilton force-pushed the ui_scaffold_routes branch 2 times, most recently from 358a44e to 5bf3c15 Compare March 19, 2021 14:29
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

ryanahamilton and others added 3 commits March 21, 2021 17:25
- fix linting with `skipLibCheck`
- fix motion warning on test with `resolutions` in `package.json`
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@ryanahamilton

Copy link
Copy Markdown
Contributor Author

Closing in favor of #14927

@ryanahamilton ryanahamilton deleted the ui_scaffold_routes branch March 24, 2021 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIP-38 Modern Web Application area:UI Related to UI/UX. For Frontend Developers. dont-merge full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add routing + scaffolding of views

3 participants