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

[web] [WIP] Mitmweb options editor content #2423

Merged
merged 8 commits into from Jul 5, 2017

Conversation

Projects
None yet
2 participants
@MatthewShao
Contributor

MatthewShao commented Jul 2, 2017

Following the pattern of settings and for the convenience of later operation, I changed the response of GET /options into something like this:

{
    anticache: {type: "bool", default: false, value: true, help: "help text"},
    intercept: {type: "str", default: null, value: null, help: "help text"},
    ....
}

Then I ported the options to the state, and made a toggle component for the option.
However, when clicking the toggle component in the option modal, it will only change the state of settings, instead of options. So we fail to switch it on and off.

@mhils @cle1000 Am I missing anything here?

@mhils

This looks good. We're getting a bit ahead of ourselves here by adding editing functionality before actually listing all options, but I guess we'll manage that. 😃 👍

it will only change the state of settings, instead of options. So we fail to switch it on and off.

I assume the option changes if you reload the page? If so, that's excellent news - that means everything is working as intended, we just do not broadcast option updates in the backend yet: web/master.py#L88. This line is called whenever the options object is updated and then issues a websocket message to all connected clients. I think we only need to add a broadcast for options here and you should see a corresponding frame in the WebSocket connection in the Chrome Dev Tools, and ultimately an update of the options state as well.

options_list.append(option)
return options_list
options_dict[k] = option
return options_dict

This comment has been minimized.

@mhils

mhils Jul 3, 2017

Member

This is a good change. 👍

@mhils

mhils Jul 3, 2017

Member

This is a good change. 👍

Show outdated Hide outdated web/src/js/components/Modal/OptionTypes.jsx
@MatthewShao

This comment has been minimized.

Show comment
Hide comment
@MatthewShao

MatthewShao Jul 5, 2017

Contributor

peek 2017-07-05 08-56
Following @mhils 's advices, I made some adjustments, here. I changed OptionTypes.jsx -> OptionMaster.jsx, which can switch from different option components according to the option type.
Now bool, str, int and choices option types were implemented, but I'm not sure about how to deal with optional str and sequence of str. Also some options relevant to file operations maybe should have they corresponding component.
The view is still so ugly now, but we could add some CSS to beautify it once we nail down the structure.

Contributor

MatthewShao commented Jul 5, 2017

peek 2017-07-05 08-56
Following @mhils 's advices, I made some adjustments, here. I changed OptionTypes.jsx -> OptionMaster.jsx, which can switch from different option components according to the option type.
Now bool, str, int and choices option types were implemented, but I'm not sure about how to deal with optional str and sequence of str. Also some options relevant to file operations maybe should have they corresponding component.
The view is still so ugly now, but we could add some CSS to beautify it once we nail down the structure.

@mhils

Awesome! Structure looks really great. For optional strings, we can probably just treat an empty message as null/None. Sequences of strings is a bit more difficult. We could maybe make it a textarea and then split by line? Anyways, we can definitely merge this already and then follow up with that.
Do you want to keep the option name and the whole <div className="menu-entry"> in the individual options, or do we want to move that to Wrapper? As a first step towards a nicer style, we should probably make this a two-column name/option layout.

@MatthewShao

This comment has been minimized.

Show comment
Hide comment
@MatthewShao

MatthewShao Jul 5, 2017

Contributor

These commits include following changes:

  1. I ditch the connect in OptionTypes, because that will make our test work easier.
  2. Move <div className="menu-entry"> to the OptionMaster( we called it Wrapper originally)
  3. Update the tests for ui as well as the backend websocket part.
  4. Add key= for components in maps()
    Please merge this PR for me if everything looks OK, then I'll open a new PR for the optional str and sequence of str components, and maybe give it a better look by adding some CSS.
Contributor

MatthewShao commented Jul 5, 2017

These commits include following changes:

  1. I ditch the connect in OptionTypes, because that will make our test work easier.
  2. Move <div className="menu-entry"> to the OptionMaster( we called it Wrapper originally)
  3. Update the tests for ui as well as the backend websocket part.
  4. Add key= for components in maps()
    Please merge this PR for me if everything looks OK, then I'll open a new PR for the optional str and sequence of str components, and maybe give it a better look by adding some CSS.
@mhils

This comment has been minimized.

Show comment
Hide comment
@mhils

mhils Jul 5, 2017

Member

🍰

Member

mhils commented Jul 5, 2017

🍰

@mhils mhils merged commit 062a58f into mitmproxy:master Jul 5, 2017

2 of 4 checks passed

codecov/patch 82.5% of diff hit (target 87.89%)
Details
codecov/project 87.88% (-0.02%) compared to f3231ed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment