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

[2.x] Removal of NProgress dependency #2045

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joetannenbaum
Copy link
Contributor

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:

  • Remove all vendor prefixes from css (all are now baseline)
  • Use classList instead of manually parsing class attributes
  • Use style property instead of manually parsing style attribute

It also allowed for tighter integration with the behavior we needed out of the component. Wins all around!

Fixes #2031

@NickSdot
Copy link

@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! 🙏

@RobertBoes
Copy link
Contributor

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

@reinink
Copy link
Member

reinink commented Oct 18, 2024

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,
Copy link
Member

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.

Copy link
Member

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

@NickSdot
Copy link

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.

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! ❤️

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.

[2.x] Should nprogress remain a dependency?
4 participants