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

feat: add visual progress indicators #5186

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

issacgerges
Copy link
Contributor

@issacgerges issacgerges commented May 28, 2024

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

No, could use guidance if they are needed

Motivation / Use-Case

Adds built-in visual progress indicators to webpack-dev-server

Breaking Changes

No breaking changes

Additional Info

minimal
circular

Copy link

linux-foundation-easycla bot commented May 28, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: issacgerges / name: Issac Gerges (0c855ac, e66ffa0)
  • ✅ login: alexander-akait / name: Alexander Akait (8780851)

@alexander-akait
Copy link
Member

Thank you for you PR, can you fix lint?

Copy link

codecov bot commented May 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.19%. Comparing base (af6bd68) to head (e66ffa0).
Report is 37 commits behind head on master.

Current head e66ffa0 differs from pull request most recent head 8780851

Please upload reports for the commit 8780851 to get more accurate results.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #5186       +/-   ##
===========================================
- Coverage   90.29%   56.19%   -34.11%     
===========================================
  Files          15       11        -4     
  Lines        1577     1413      -164     
  Branches      601      555       -46     
===========================================
- Hits         1424      794      -630     
- Misses        140      507      +367     
- Partials       13      112       +99     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@issacgerges
Copy link
Contributor Author

Should be fixed. @alexander-akait the re-run of build:types seems to have stripped all comments out. let me know if theres another way to run that command that preserves them and I can do.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

I am fine with it, but I am afraid some developers still can use our client in old browsers, so shadow dom can't work, let's add a check and don't run this code

@issacgerges
Copy link
Contributor Author

@alexander-akait will do, sorry for moving so slow on this.

As an alternative, we could just expose some explicit API (or tell folks to lean on the postMessage you're already doing with progress) and leave implementing the visual parts to people who want them. What do you think?

@alexander-akait
Copy link
Member

@issacgerges Yeah, we can, but I am fine with your solution, just let's add additional check to avoid problems with old browsers

@issacgerges
Copy link
Contributor Author

issacgerges commented Jun 4, 2024

Updated

  • Added a guard
  • Made the options "linear" and "circular" for consistency
  • Combined both into a single custom element and simplified it a bit

@@ -236,6 +239,18 @@ const onSocketMessage = {
);
}

if (isProgressSupported()) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's move defineProgressElement after this check to avoid extra checks and have customElements.define("wds-progress", WebpackDevServerProgress); only in this function

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks very good, let's do one small improvement and we can merge

alexander-akait
alexander-akait previously approved these changes Jun 5, 2024
@alexander-akait
Copy link
Member

Thank you, good work ⭐

@alexander-akait alexander-akait merged commit a8f40b7 into webpack:master Jun 5, 2024
39 checks passed
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.

None yet

2 participants