Improvements

[milewski]

Hi i made a few improvements on the code this includes my previous PR, I will close that one

[gobrightspot]

Sorry, there are too many changes in here to merge this, I don't agree with some of them either. Thanks for spotting the missing Nova class in the use block. In order to save yourself some time in future, maybe submit an issue first to discuss the changes.

[milewski]

I think what I changed was pretty standard... like all the other packages follows it.. whats exactly you think is is not correct?

[gobrightspot]

You removed the DetachedActions class, so now end users of the package don't opt-in anymore, we force our scripts and styles on them when then install the package. That's not necessary.

Using the default conventions for the button colors will make all buttons look the same, that's not what we were trying to do. I think it's important that the create button remain the 'primary' action but we added a css class to the button so people can theme that as they need. Unfortunately, Nova currently does not have a 'secondary' action color.

I don't agree that the way you handled css is how other packages do it.

I'm good with the other changes (except the ones that were caused by the above)

[milewski]

You removed the DetachedActions class, so now end users of the package don't opt-in anymore, we force our scripts and styles on them when then install the package. That's not necessary.

Why someone would want to install this and not use it? I feel that step is unnecessary

Using the default conventions for the button colors will make all buttons look the same, that's not what we were trying to do. I think it's important that the create button remain the 'primary' action but we added a css class to the button so people can theme that as they need. Unfortunately, Nova currently does not have a 'secondary' action color.

I feel using the default is better than using something totally out of style.. currently, the theme I made for my clients and of many others that has posted issues on my packages usually build their own themes, in my case I have bright colors, when using this package it adds a button that really stands out... black/white ... the first question the client might ask is why does this button seems broken? ...

I don't agree that the way you handled css is how other packages do it.

Having an extra request to fetch a .css really adds up... especially with nova ... I have around 30 packages if all of them added 1css 1js that would be 60 requests to load a small simple page.. inlining it inside the vue file saves your user 1 less request.

[gobrightspot]

Hey, first off, I want to say thank you for spending time trying to improve the package. It is appreciated.

One thing to consider, different people have encountered different problems with 3rd party tools, one of the things we’ve encountered is that during different parts of the year, our UI in Nova needs to change.

Why someone would want to install this and not use it? I feel that step is unnecessary.

One reason we’ve done this is so that we can avoid bringing in the assets and functionality during periods in the year where our end users are not allowed to run these actions, having the ability to conditionally include the tool is important for us.

I feel using the default is better than using something totally out of style.. currently, the theme I made for my clients and of many others that has posted issues on my packages usually build their own themes, in my case I have bright colors, when using this package it adds a button that really stands out... black/white ... the first question the client might ask is why does this button seems broken?

I’m not sure I agree with this and we’ve provided a way to customize the button too. So there’s nothing stopping you from theming. That being said, I understand your argument for a different default. If you feel strongly on this, please create a PR that only addresses this specific issue, limit the scope of the change. I’d like to let it sit for a little bit with our team and also if anyone happens across the PR, they may leave an opinion on it too.

Having an extra request to fetch a .css really adds up... especially with nova ... I have around 30 packages if all of them added 1css 1js that would be 60 requests to load a small simple page.. inlining it inside the vue file saves your user 1 less request.

This may be solved depending on how we handle the above.

[milewski]

One reason we’ve done this is so that we can avoid bringing in the assets and functionality during periods in the year where our end users are not allowed to run these actions, having the ability to conditionally include the tool is important for us.

Well, it's definitely not a good way of doing this.. Although most of the users will not be smart enough to inspect the website or try to do something beyond what they are seeing... but the API would still be active.. anyone with a bit of knowledge would be able to call your disabled actions anyways.. not that this matters but if you running something critical someone messing with API without authorization can be dangerous...

Besides, it is also possible to disable the assets of a specific package just by unsetting it:

unset(Nova::$styles['package-name']);

Not ideal neither but the 1% that may need to do this that's would be acceptable I guess

But regardless I am okay to use my fork I will do a few more improvements there to align with the latest specs from nova 3.5 and that's it

[gobrightspot]

But regardless I okay to use my fork I will do a few more improvements there to align with the latest specs from nova 3.5 and that's it

You do you buddy 👍🏻