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.

Sign up for GitHub

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

Jump to bottom

URL navigation #451

Merged
merged 1 commit into from Sep 8, 2020
Merged

URL navigation #451

merged 1 commit into from Sep 8, 2020

Conversation

dcm
Copy link
Contributor

@dcm dcm commented Jun 28, 2020 •
edited

NOTE: Internally we have one change that depends on this PR, which is necessary before making a new release.

  • add ability to debug using a sample report
  • require trailing slashes on URLs
  • stop page from complaining about missing manifest.json
  • Silence loud chrome warnings in the devtools console by switching our
    fontawesome CDN tag to a crossorigin-friendly tag
  • add lodash
  • .eslintrc.json: Make eslint allow comments up to 120 characters
  • AssertionPane.js: export aphrodite CSS for reuse
  • CenterPane.jsx: rewrite of GetCenterPane function in reportUtils.js
  • state.js: major revamp of UI state management
    • switch to redux to avoid unstable_observedBits log messages
  • remove optional dependencies and upgrade react-hooks-testing-library
    to the non-deprecated version @testing-library/react-hooks
  • revert improvements
  • fix link style, remove extraneous action from store serializableCheck,
    handle testcase onClick edgecase, remove es imports
  • remove uriComponentCodec, make imports easier
  • make connect easier
  • simplify build girth, simplify rigidity, simplify & segregate redux
  • remove as much hard to read code as is possible
  • deal with special .env test rules
  • make travis env vars work
  • setup new API URL - env var scheme

@dcm dcm changed the title Squashed: URL navigation Jun 28, 2020
@dcm
Copy link
Contributor Author

dcm commented Jun 28, 2020

There are not 13,000 added lines that need to be reviewed...

I moved our document mocks and snaps to different folders, hence these line counts should not cause panic:

This is a WIP at the moment since I still need to prune some files and code from what we see at 0669a3f.

@dcm dcm mentioned this pull request Jun 28, 2020
Closed
yuxuan-ms
yuxuan-ms reviewed Jul 1, 2020
View reviewed changes
Copy link
Contributor

@yuxuan-ms yuxuan-ms left a comment

Choose a reason for hiding this comment

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

I just add some minor comments. Needs a demo to review the changes step by step.

testplan/web_ui/testing/public/index.html Show resolved Hide resolved
testplan/web_ui/testing/src/AssertionPane/AssertionHeader.js Outdated Show resolved Hide resolved
testplan/web_ui/testing/src/Common/ErrorCatch.jsx Outdated Show resolved Hide resolved
testplan/web_ui/testing/src/Common/SwitchRequireSlash.jsx Outdated Show resolved Hide resolved
testplan/web_ui/testing/src/Common/uriComponentCodec.js Outdated Show resolved Hide resolved
testplan/web_ui/testing/src/Report/BatchReport/components/CenterPane/index.jsx Outdated Show resolved Hide resolved
testplan/web_ui/testing/src/Report/BatchReport/components/PrintButton.jsx Outdated Show resolved Hide resolved
testplan/web_ui/testing/src/Report/BatchReport/state/__tests__/reportSlice.test.js Outdated Show resolved Hide resolved
testplan/web_ui/testing/src/Report/BatchReport/state/reportSelectors.js Outdated Show resolved Hide resolved
testplan/web_ui/testing/src/__tests__/documents/index.js Outdated Show resolved Hide resolved
@dcm dcm marked this pull request as ready for review July 14, 2020 08:34
@dcm dcm requested a review from Pyifan as a code owner July 14, 2020 08:34
@dcm
Copy link
Contributor Author

dcm commented Jul 14, 2020

Right now the new component BatchReportBeta could be merged and nobody would be harmed, even if it has massive underlying bugs.

(it doesn't.)

This component is used in place of the current BatchReport only if the user verbosely opts-in by visiting /testplan/beta/1004/uid234 rather than the normal /testplan/uid234.

Pyifan
Pyifan reviewed Jul 16, 2020
View reviewed changes
Copy link
Contributor

@Pyifan Pyifan left a comment

Choose a reason for hiding this comment

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

I'm only half way through the PR. to be continued...

testplan/web_ui/testing/.gitignore Outdated Show resolved Hide resolved
testplan/web_ui/testing/src/Common/utils.js Show resolved Hide resolved
testplan/web_ui/testing/src/Report/BatchReportBeta/components/AutoSelectRedirect.jsx Show resolved Hide resolved
testplan/web_ui/testing/src/Report/BatchReportBeta/components/AutoSelectRedirect.jsx Outdated Show resolved Hide resolved
testplan/web_ui/testing/src/Report/BatchReportBeta/components/AutoSelectRedirect.jsx Outdated Show resolved Hide resolved
testplan/web_ui/testing/src/Report/BatchReportBeta/components/AutoSelectRedirect.jsx Outdated Show resolved Hide resolved
testplan/web_ui/testing/src/Report/BatchReportBeta/components/DocumentationButton.jsx Show resolved Hide resolved
testplan/web_ui/testing/src/Report/BatchReportBeta/components/HelpButton.jsx Show resolved Hide resolved
testplan/web_ui/testing/src/Report/BatchReportBeta/components/InfoButton.jsx Show resolved Hide resolved
@dcm
Copy link
Contributor Author

dcm commented Jul 20, 2020

Travis tests are failing due to a known Jest issue, workaround in process...

Pyifan
Pyifan reviewed Jul 22, 2020
View reviewed changes
testplan/web_ui/testing/src/state/appSlice.js Outdated
const API_BASE_URL = process.env.REACT_APP_API_BASE_URL;

/** always false in production */
const boolIfDev = val => __DEV__ ? !!val : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Name this falseIfProd will make the code easier to read, but i wish we don't need this at all.

Pyifan
Pyifan reviewed Jul 22, 2020
View reviewed changes
testplan/web_ui/testing/src/Report/BatchReportBeta/state/reportSlice.js
} else {
state.isFetchCancelled = false;
state.fetchError = action.error;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems fetchReport has 3 different state: pending/fulfilled/rejected, and we are mapping it 2 flags: isFetching/isFetchCancelled + a fetchError field. Can we simplify things here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how

@dcm dcm force-pushed the off-pr-ops branch 7 times, most recently from d207b64 to 9179d42 Compare August 12, 2020 07:12
@dcm dcm force-pushed the off-pr-ops branch 2 times, most recently from 2e100d9 to b22ee2d Compare September 2, 2020 07:13
@dcm
Squashed:
2dd3cd2 
* add ability to debug using a sample report
* require trailing slashes on URLs
* stop page from complaining about missing manifest.json
* Silence loud chrome warnings in the devtools console by switching our
  fontawesome CDN tag to a crossorigin-friendly tag
* add lodash
* .eslintrc.json: Make eslint allow comments up to 120 characters
* AssertionPane.js: export aphrodite CSS for reuse
* CenterPane.jsx: rewrite of GetCenterPane function in reportUtils.js
* state.js: major revamp of UI state management
  * switch to redux to avoid unstable_observedBits log messages
* remove optional dependencies and upgrade react-hooks-testing-library
  to the non-deprecated version @testing-library/react-hooks
* revert improvements
* fix link style, remove extraneous action from store serializableCheck,
  handle testcase onClick edgecase, remove es imports
* remove uriComponentCodec, make imports easier
* make connect easier
* simplify build girth, simplify rigidity, simplify & segregate redux
* remove as much hard to read code as is possible
* deal with special .env test rules
* make travis env vars work
* setup new API URL - env var scheme
@dcm
Copy link
Contributor Author

dcm commented Sep 2, 2020 •
edited

The API URL bit we discussed is implemented and is explained here.

@yuxuan-ms we okay to merge?

@yuxuan-ms yuxuan-ms merged commit 0809c96 into morganstanley:master Sep 8, 2020
@yuxuan-ms yuxuan-ms mentioned this pull request Nov 11, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pyifan Pyifan Pyifan left review comments

@yuxuan-ms yuxuan-ms yuxuan-ms approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dcm @Pyifan @yuxuan-ms