-
-
Notifications
You must be signed in to change notification settings - Fork 434
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
[2.x] Removal of NProgress dependency #2045
base: master
Are you sure you want to change the base?
Conversation
@joetannenbaum thanks for taking this on! To be honest, I kinda was hoping to also get rid of some bundle size and unused ballast as a result of my question/proposal. 😅 Would you mind to gimme an idea of why having https://github.com/inertiajs/progress as an optional thing wouldn't be sufficient? Afaik that was the plan for years, no? Thank you in advance! 🙏 |
Another nice option would be if the progress stuff is treeshakable, then it wouldn't end up in the bundle if the option isn't enabled |
Personally don't think it's unreasonable for Inertia to ship with a built-in progress bar, and I like how that keeps the installation story simpler. Plus it's less than 400 lines of code, so not a big deal either way. |
speed: 200, | ||
trickle: true, | ||
trickleSpeed: 200, | ||
showSpinner: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of wish we could just remove the spinner entirely. Wish I knew how many people were actually using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently some people like it: #96
Almost impossible to argue against the legend himself, though. 😅 However, I politely disagree. The installation story is really not that much simpler. And, a 400 lines here, a 400 line there, these that - you know where I am coming from. Anyway, I understand and accept that is your call! I'd appreciate if you would take Roberts comment above and #1953 into account then. Thanks for all your hard work! ❤️ |
As mentioned in #2031,
nprogress
has been inactive for quite a long time and with the 2.x release it's probably smart to remove it as a dependency and bring it in house.This also allowed for a refactor, bringing it up to more modern web standards. Among the improvements:
classList
instead of manually parsing class attributesstyle
property instead of manually parsing style attributeIt also allowed for tighter integration with the behavior we needed out of the component. Wins all around!
Fixes #2031