Add url interception without listening #133

amitmbee · 2018-06-15T11:54:50Z

Adds ability to intercept requests without explicitly having to listen to them

Add URL Modal:

After the URL is added for interception:

Add ability to edit URL's inline by changing the URL fields

more screenshots (Design by @cgirish )

revathskumar

Attach some screenshots and add test cases.

revathskumar · 2018-06-18T05:01:11Z

app/__tests__/popup.test.jsx

+          .find("button")
+          .at(0)
+          .hasClass("button-start-listening")
+      ).toEqual(true);


use toBeTruthy()

revathskumar · 2018-06-18T05:03:34Z

app/components/AddRuleModal.tsx

+        type="text"
+        name="modal-url"
+        id="url-input-modal"
+        onChange={e => props.updateModalUrl(e.target.value)}


onChange={props.updateModalUrl}.
updateModalUrl will receive event object as first argument and do the e.target.value inside updateModalUrl.

Advantage: avoiding function declaration on reach render.

Had missed this point^^. Thanks for reminding. Better to use this pattern everytime we pass event/event properties

revathskumar · 2018-06-18T05:04:35Z

app/components/AddRuleModal.tsx

+export const AddRuleModal: React.SFC<{}> = props => {
+  return (
+    <Modal show={props.showModal} handleClose={props.handleClose}>
+      <label htmlFor="modal-url">URL</label>


for should be associated with id not name

revathskumar · 2018-06-18T05:05:15Z

app/components/AddRuleModal.tsx

+      <input
+        className="form-control"
+        type="text"
+        name="modal-url"


name like modal-url can be better. no need to have modal in context for the name of this input.
better will be request_url.

avoid - (hypen) for names.

revathskumar · 2018-06-18T05:06:23Z

app/components/AddRuleModal.tsx

+        id="url-input-modal"
+        onChange={e => props.updateModalUrl(e.target.value)}
+      />
+      <label htmlFor="modal-method">Method</label>


for should be associated with id not name

revathskumar · 2018-06-18T05:06:54Z

app/components/AddRuleModal.tsx

+          defaultValue="GET"
+          value={props.modalMethod}
+          className="modal-method"
+          onChange={e => props.updateModalMethod(e.target.value)}


onChange={props.updateModalUrl}. for the same reason above.

revathskumar · 2018-06-18T05:08:21Z

app/components/ModalWrapper/index.tsx

@@ -0,0 +1,14 @@
+import * as React from "react";
+export const Modal = ({ handleClose, show, children }) => {
+  const showHideClassName = show ? "modal display-block" : "modal display-none";


no need to switch the css classes.
Instead either we render or no render the component.

revathskumar · 2018-06-18T05:09:05Z

app/containers/Popup.tsx

@@ -28,6 +31,11 @@ const isChromeUrl = (url: string) => {
  return CHROME_URL_REGEX.test(url);
 };
 export class Popup extends React.Component<POPUP_PROPS & DispatchProps, {}> {
+  state = {


move to redux.

Since its a container component and the state defined here is used just to show/hide the modal window(not much complication regarding state involved), should we move this to redux ?

…resence of className

…y function creation by passing event object instead of target.value

… property to show/hide modal window

…e - Stored modal window state across tabs for data persistence

revathskumar · 2018-06-22T06:53:31Z

@cgirish Can you take a look into this for a better design?
Should we really need to go with Modal any alternate approach?

@Amit-MB if we are deciding to go with Modal we need a backdrop.

revathskumar

Need validation for the user input URL.

revathskumar · 2018-06-22T06:46:09Z

app/__tests__/AddRuleModal.test.jsx

+      expect(wrapper.find("Modal")).toHaveLength(1);
+    });
+    test("Contains a input, select and a button element", () => {
+      expect(wrapper.find("input")).toBeTruthy();


AFAIK, this won't work. wrapper.find("input") always returns an object. so we have to check for the length to make sure the element exists

revathskumar · 2018-06-22T06:50:33Z

app/__tests__/AddRuleModal.test.jsx

+  showModal: false,
+  modalMethod: "GET",
+  addRequest: jest.fn(),
+  modalUrl: "www.codemancers.com",


Don't modal data based on UI. what if tomorrow we decided to not go with Modal?
the name become meaningless and since data is used everywhere, renaming will impact a lot of places.

Yes. ^^ that makes sense. Will change names.

revathskumar · 2018-06-22T06:54:40Z

app/actions/index.ts

-  return { type: INITIALISE_DEFAULTS, payload: { currentTab, currentUrl, interceptStatus } };
+  return {
+    type: INITIALISE_DEFAULTS,
+    payload: { currentTab, currentUrl, interceptStatus }


Since we already have initialState, why we need to send the payload?

Although initialState is defined in the reducer, we dont have any data about the currentTabId, currentTabUrl. Therefore, I dispatch this action as soon as the popup is opened so that we can store information based on tabId. Better to rename this as currentTabId, currentTabUrl.

I also dispatch interceptStatus with empty string so that after each time the popup is opened, the Success or Error message gets erased.

revathskumar · 2018-06-22T06:56:12Z

app/components/ModalWrapper/index.tsx

@@ -0,0 +1,16 @@
+import * as React from "react";
+export const Modal = ({ handleClose, show, children }) => {
+  if (!show) {


This condition shouldn't be a part of this component.

{showModal && <Modal {...props} />}

…lements to assert their presence

…d for AddRuleModal component

…t method in Popup.tsx

…lid and inValid urls

…ate changes in reducer

…ercept url on onChange event.

revathskumar · 2018-07-05T07:48:53Z

@cgirish @Amit-MB Are we going forward with Modal itself? Or any better approach we are looking?

revathskumar · 2018-07-05T07:50:13Z

@Amit-MB Also please put the error message of URL validaiton near the text box itself. Don't put those error message in global error.

amitmbee · 2018-07-05T07:55:56Z

I have also added the ability to edit url's in this PR (see 4th screenshot in PR description). Let's for @girish and check if there are any better design approaches.

@revathskumar , error message of URL validaiton near the text box itself,good idea than showing as global error, i will make necessary changes required and push 👍

…tton is clicked

…specs accordingly

…esome

revathskumar · 2018-07-17T13:44:12Z

app/components/AddRuleModal.tsx

+  updateAddRequestMethod: (value: string, tabId: number) => void;
+  updateAddRequestUrl: (value: string, tabId: number) => void;
+}
+export const AddRuleModal: React.SFC<AddRuleModalProps> = props => {


Make it extend from Component.
As already discussed, if we need instance methods never use Function component.

We are using functions(defined internally) and not methods or lifecycle methods as well. Do we really need a stateful component here ?

functions defining in render method is not good, this is similar to it.
This will define all the methods again and again when each time it renders.

By using Component doesn't mean stateful. we are not using internal state.

revathskumar · 2018-07-17T13:44:39Z

app/components/AddRuleModal.tsx

+        </button>
+        <button
+          className="btn btn-primary btn-add-rule"
+          onClick={() => {


Avoid inline method and move to instance method.

revathskumar · 2018-07-17T13:45:19Z

app/components/AddRuleModal.tsx

+
+  const handleClose = () => {
+    //erase the previously set error message on each re-render
+    props.addRuleErrorNotify(``, props.tabId);


Why using backticks?

revathskumar · 2018-07-17T13:45:24Z

app/components/AddRuleModal.tsx

+    props.addRuleErrorNotify(``, props.tabId);
+    //reset the url to empty string and request method to "GET"
+    props.updateAddRequestUrl(``, props.tabId);
+    props.updateAddRequestMethod(`GET`, props.tabId);


revathskumar · 2018-07-17T13:45:28Z

app/components/AddRuleModal.tsx

+    //erase the previously set error message on each re-render
+    props.addRuleErrorNotify(``, props.tabId);
+    //reset the url to empty string and request method to "GET"
+    props.updateAddRequestUrl(``, props.tabId);


revathskumar · 2018-07-17T13:47:07Z

app/components/RequestList.tsx

  updateInterceptorStatus?: (tabId: number) => void;
  fetchResponse: React.MouseEvent<HTMLSpanElement>;
  handleSwitchToggle: any;
+  handleChangeUrl: any;


…nent

…alue instead of value to prevent cursor jump issue

revathskumar · 2018-07-18T19:44:05Z

This will fix the failing test

    test ('on clear button click, should trigger props.clearFields', () => {
      let localProps = createTestProps ();
      wrapper = mount (<Popup {...localProps} />);
      wrapper.find ('.btn-clear').last ().simulate ('click');
      expect (localProps.clearFields).toBeCalledWith (1);
    });

… with tests

…mponent

revathskumar

LGTM

amitmbee added 6 commits June 15, 2018 15:27

Add ability to list intercept unlistened URL's through a modal form

09bf7eb

Modal buttons styling changes

2053ef7

Convert modal method input from type=text to <option>

81dbd15

Style cursor to be pointer for modal buttons

4a275de

Extract modal elements to a seperate react component

0277c57

fix failing tests

712889c

amitmbee requested a review from revathskumar June 15, 2018 11:54

revathskumar suggested changes Jun 18, 2018

View reviewed changes

amitmbee added 8 commits June 18, 2018 11:25

Jest change: Use toBeTruthy() instead of toEqual(true) to check for p…

41efc82

…resence of className

Link <label> for attribute to id's instead of name. Avoid un-necessar…

e8d2418

…y function creation by passing event object instead of target.value

Use event object to handle onChange events/setState in Popup component

627d64b

Render null if modalShow is set to false instead of using CSS display…

4c7e751

… property to show/hide modal window

Make a function call on onClick event trigger

8e8c959

Styling changes due to removed un-necessary <div> tag

d91c248

Add test cases for AddRuleModal component

d6ae08d

Move modal properties redux store/Dispatch actions instead of setStat…

74db7a2

…e - Stored modal window state across tabs for data persistence

revathskumar suggested changes Jun 22, 2018

View reviewed changes

amitmbee added 5 commits June 22, 2018 14:49

Better names for addUrl parameters,fix tests to check for length of e…

9a63da7

…lements to assert their presence

Validate URL before adding it for interception

9f0e5da

Add ts defns, defaultProps and removed defaultValue for request-metho…

74d96d4

…d for AddRuleModal component

Fix applying ts defns properly, remove unwanted params from addReques…

d90851d

…t method in Popup.tsx

Fix test : Verify proper function calls to be made in case of both va…

055a932

…lid and inValid urls

amitmbee requested a review from cgirish June 29, 2018 05:24

amitmbee added 6 commits June 29, 2018 11:56

Add ability to list intercept unlistened URL's through a modal form

3b07186

Modal buttons styling changes

6b26f18

Convert modal method input from type=text to <option>

b52a549

Style cursor to be pointer for modal buttons

ad7e2b2

Extract modal elements to a seperate react component

32d0de2

fix failing tests

162f854

amitmbee added 2 commits June 29, 2018 15:12

Make all url's content-editable with onChange handler to make approri…

534dd9e

…ate changes in reducer

Move content-editable section to collapsed div. Ability to change int…

8a2d9a9

…ercept url on onChange event.

amitmbee force-pushed the Add-url-interception-without-listening branch from 47e30fe to 8a2d9a9 Compare July 5, 2018 10:12

amitmbee and others added 9 commits July 5, 2018 15:52

Merge branch 'master' into Add-url-interception-without-listening

bb0dac9

add missing comma in object

8305c46

Show error for addUrl in modal itself, add better accessibility features

76da58c

Fix tests: add more tests for when modal is closed or when add url bu…

bcf38b8

…tton is clicked

Update the UI design of Add Rule Modal

8b83dac

Merge branch 'master' into Add-url-interception-without-listening

e3c489c

Realign add rule buttons with other clear and intercept buttons, fix …

0688d23

…specs accordingly

Fix tests by adding missing classnames in component

6640f64

Readme : Add attribution and license link for icons used from font-aw…

c09539d

…esome

revathskumar suggested changes Jul 17, 2018

View reviewed changes

amitmbee added 5 commits July 17, 2018 19:36

Replace backticks with quotes - better readbility

c3cd3b9

Extract inline anonymous function defn, extract it to a defined function

773460e

Remove duplicate handleChangeUrl ts definition from RequestList compo…

4b0eccd

…nent

Convert AddRuleModal functional to pure component - also use defaultV…

966fb51

…alue instead of value to prevent cursor jump issue

Attempt to fix tests

c541f6f

amitmbee added 3 commits July 19, 2018 07:01

fix failing tests

9ff013d

Remove unwanted className btn-clear for Intercept button: Conflicting…

415fff2

… with tests

Remove .first() in tests as the className is used only once in the co…

3890863

…mponent

revathskumar approved these changes Jul 23, 2018

View reviewed changes

revathskumar merged commit 154edbd into master Jul 23, 2018

revathskumar deleted the Add-url-interception-without-listening branch July 23, 2018 08:12

amitmbee mentioned this pull request Jul 23, 2018

Add a interceptor rule/mock response even if the request isn't listed #112

Closed

revathskumar mentioned this pull request Jul 24, 2018

Feature timeline #143

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add url interception without listening #133

Add url interception without listening #133

amitmbee commented Jun 15, 2018 •

edited

Loading

revathskumar left a comment

revathskumar Jun 18, 2018

revathskumar Jun 18, 2018

amitmbee Jun 18, 2018

revathskumar Jun 18, 2018

revathskumar Jun 18, 2018

revathskumar Jun 18, 2018

revathskumar Jun 18, 2018

revathskumar Jun 18, 2018

revathskumar Jun 18, 2018

amitmbee Jun 18, 2018

revathskumar commented Jun 22, 2018

revathskumar left a comment

revathskumar Jun 22, 2018

revathskumar Jun 22, 2018

amitmbee Jun 22, 2018

revathskumar Jun 22, 2018

amitmbee Jun 22, 2018

revathskumar Jun 22, 2018

revathskumar commented Jul 5, 2018

revathskumar commented Jul 5, 2018

amitmbee commented Jul 5, 2018

revathskumar Jul 17, 2018

amitmbee Jul 17, 2018

revathskumar Jul 17, 2018

revathskumar Jul 17, 2018

revathskumar Jul 17, 2018

revathskumar Jul 17, 2018

revathskumar Jul 17, 2018

revathskumar Jul 17, 2018

revathskumar commented Jul 18, 2018

revathskumar left a comment

Add url interception without listening #133

Add url interception without listening #133

Conversation

amitmbee commented Jun 15, 2018 • edited Loading

revathskumar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revathskumar commented Jun 22, 2018

revathskumar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revathskumar commented Jul 5, 2018

revathskumar commented Jul 5, 2018

amitmbee commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revathskumar commented Jul 18, 2018

revathskumar left a comment

Choose a reason for hiding this comment

amitmbee commented Jun 15, 2018 •

edited

Loading