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.

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

Rewrite UI and add Jest tests #94

Closed
wants to merge 7 commits into from
Closed

Conversation

robmosca
Copy link
Contributor

@robmosca robmosca commented Sep 10, 2023

THIS PR IS STILL A DRAFT. NOT READY FOR REVIEW.

DISCLAIMER
I created this PR on top of #89.
This PR includes all the changes of

It addresses issue #22.

With the accumulation of the three PRs this one is quite big. I would normally break it down in several smaller PRs but I considered including all changes in one, in the hope it would accelerate the process. I would be happy of breaking the PR down in smaller chunks if we can agree on a reviewing plan.

MOTIVATION
The import-map-overrides code is currently not covered by Jest tests. This makes updating the project rather risky, as any change needs to be manually tested to make sure no regressions are introduced and the manual process might be missing potential errors.
Also, the interface looks rather outdated with a spartan styling of components. In addition to that, the code still uses class components, while the practice nowadays is to use functional components, for their simplicity and ease of maintenance.
Finally, the repository does not include any automation for deployment, a thing that has led in the past to problems for the deployed version of other packages in the project.
This PR tries to remediate all these problems.

CHANGES INCLUDED
This PR includes changes from other 2 PRs (see above). We will summarize here the list of changes introduced:

  1. Added Jest tests covering the API (see Add Jest tests for api #87)
  2. Added publishing automation using the changesets GitHub release action (see Add tests and automated publishing the package to NPM #89)
  3. Rewrote all preact components as functional components
  4. Refactored the logic for calculating the different types of modules in a context (ImportMapOverridesContext) which allows sharing information to several components in the hierarchy, leading to a more responsive UI where changes can be reflected in several places. For example, the button now can change color to indicate that there are pending changes that need a reload. This also allows better encapsulation and testing of that logic.
  5. Added Jest tests covering all UI components. The current test coverage for the project can be seen in this image:
    Screenshot 2023-09-10 at 18 57 24
  6. Revamped the UI:
    • Modernized and reorganized the element's style
    • Added action items in the table of overridden modules, to simplify interaction with them
    • Introduced light/dark themes matching the browser theme
    • Fixed minor inconsistencies. For example, while the UI shows a reload button for modules in nextOverriddenModules it does not do the same for modules in pendingRefreshDefaultModules, even if they also would require a reload to become effective).
    • The trigger button now only appears when the popup is closed. Its color reflects the status of modules: grey if there is no override, yellow if there is an override pending a reload, red if there is any active override.

You can see how the interface looks like in the following video and images:

imo.mp4

Screenshot 2023-09-10 at 19 26 42
Screenshot 2023-09-10 at 19 26 58
Screenshot 2023-09-10 at 19 27 27
Screenshot 2023-09-10 at 19 27 51
Screenshot 2023-09-10 at 19 28 06
Screenshot 2023-09-10 at 19 28 23
Screenshot 2023-09-10 at 19 28 37

@robmosca
Copy link
Contributor Author

I will close this PR and reopen multiple PRs addressing the change bit by bit.

@robmosca robmosca closed this Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant