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

[WIP] [web] Mitmweb options editor UI #2416

Merged
merged 14 commits into from Jul 1, 2017

Conversation

Projects
None yet
3 participants
@MatthewShao
Contributor

MatthewShao commented Jun 27, 2017

  • Add reducer for the modal
  • Implement Modal Component
  • Add tests
@MatthewShao

This comment has been minimized.

Show comment
Hide comment
@MatthewShao

MatthewShao Jun 28, 2017

Contributor

I implemented the modal component with the help of the work done by @cle1000 last summer. However, my implementation might be immature, so please feel free to comment on it.
Here's how it looks now:
peek 2017-06-28 18-39
The new files cause the coverage drop, if this structure is approved, I will add some tests for the new files then.

Contributor

MatthewShao commented Jun 28, 2017

I implemented the modal component with the help of the work done by @cle1000 last summer. However, my implementation might be immature, so please feel free to comment on it.
Here's how it looks now:
peek 2017-06-28 18-39
The new files cause the coverage drop, if this structure is approved, I will add some tests for the new files then.

@mhils

Looks good so far - no Bootstrap JS required 😄.

I vote to merge this once the outstanding comments are accepted (and maybe tests are added), so that we have cleaner PRs afterwards.

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

This comment has been minimized.

Show comment
Hide comment
@MatthewShao

MatthewShao Jun 30, 2017

Contributor

There was a discussion between me and @cle1000 about the Modal component, he suggests that we should have a generic component for the modal, which should contains all the redundant information. Besides, we should make the Modal.jsx as simple as possible. Therefore I reconsider the structure of the modal component and then make some changes:

  1. Modal.jsx: be responsible for switching between different modal components, and turning them on and off.
  2. ModalLayout.jsx: returns a generic component, containing <div className='modal-backdrop'> for the dark background and div className='modal'for the layout.
  3. OptionModal.jsx: returns the header, body, footer which below to Modal, also takes care of the logic of fetching and updating options. We need to connect it to the state.
  4. ModalList.jsx: exporting all the available modals. Because we select the modal component by function name in Modal.jsx, so we need to give a specific name to every modal component. However, if we want to connect the component to the state, the connect function can not return a function with specific name. My solution is wrapping up the component in the ModalList.jsx, so we could query the modal by name while still have it connect to the state.
Contributor

MatthewShao commented Jun 30, 2017

There was a discussion between me and @cle1000 about the Modal component, he suggests that we should have a generic component for the modal, which should contains all the redundant information. Besides, we should make the Modal.jsx as simple as possible. Therefore I reconsider the structure of the modal component and then make some changes:

  1. Modal.jsx: be responsible for switching between different modal components, and turning them on and off.
  2. ModalLayout.jsx: returns a generic component, containing <div className='modal-backdrop'> for the dark background and div className='modal'for the layout.
  3. OptionModal.jsx: returns the header, body, footer which below to Modal, also takes care of the logic of fetching and updating options. We need to connect it to the state.
  4. ModalList.jsx: exporting all the available modals. Because we select the modal component by function name in Modal.jsx, so we need to give a specific name to every modal component. However, if we want to connect the component to the state, the connect function can not return a function with specific name. My solution is wrapping up the component in the ModalList.jsx, so we could query the modal by name while still have it connect to the state.

@MatthewShao MatthewShao merged commit f3231ed into mitmproxy:master Jul 1, 2017

4 checks passed

codecov/patch 92.1% of diff hit (target 87.87%)
Details
codecov/project 87.89% (+0.01%) compared to d58abc9
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