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

Solves #37 - Support for Umbraco 8 #38

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

Conversation

enkelmedia
Copy link

This PR adds support for Umbraco 8.4+ (since they changed the value converter-implementation I choose to go with the latest version).

We've also removed external dependencies to make the package it own "unit".

I have not updated any meta data for release/build for nuget/our.umbraco and not changed any of the markdown for readme etc.

Let me know if you would like any changes

@enkelmedia enkelmedia changed the title 37 Solves #37 - Support for Umbraco 8 Jan 20, 2020
@abjerner
Copy link
Member

@enkelmedia thanks a lot - I'll find some time to look at your changes 👍

(and sorry for not answering your previous issues and PRs).

@abjerner
Copy link
Member

Hi @enkelmedia,

I've now looked through your changes, and have some comments:

  • Project format
    The project uses the old .csproj format. For most of my own packages, I have switched to the newer SDK format. The new format helps keeping things simple, so ideally we should also have that here.

    Is that something you can look into? You can see an example here. Let me emphasize that this is nice to have, but not necessary to get your PR merged. So if you prefer, I'd be happy to have a look at it instead once your PR has been merged.

  • Assembly conflicts
    The Visual Studio project has some assembly conflicts - possibly after upgrading. I'm not really sure how those will affect the project, but switching to the SDK format as mentioned above should fix that.

  • Datepicker
    The property editor allows you to specify certain holidays with different opening hours, but the datepicker is broken, causing the backoffice to reload. I can the problem lies around this line as Umbraco 8 handles overlays a bit different than in Umbraco 7. Is this something you can look into?

  • Colors
    The colors are a bit off here and there. For instance the grey background is slightly lighter than it should be, and the same goes for the blue color for the +.

    You can see the variables.less file to see the different colors used by Umbraco. For the .OpeningHours .panel-default > .panel-heading CSS selector, you may be able to switch to @gray-9 for the border, and @brownGrayLight for the background. And then @blueExtraDark for .OpeningHours a.add-time.

  • Skybrud.Essentials
    I'd like to hear a bit more about your reasons for removing the Skybrud.Essentials dependency (it may be fine to do so).

    I think most of the extension methods origins from my Skybrud.Social package. At some point I then also needed the same logic in some of our other packages, and then suddenly we had a handful of packages containing the exact same logic. And in our client projects, we just ended up using the extension methods from the package that Visual Studio / Resharper suggested first (so in a way quite random). And when we had bug fixes and improvements, it made it hard to make sure we had updated all implementations or - just using the implementation that had been fixed. So as a result of this, the Skybrud.Essentials package was created to put some of this logic in a single place. Most of our other packages therefore has Skybrud.Essentials as a dependency.

    I think the reasons we had for creating Skybrud.Essentials also serves as an argument for keeping it here as a dependency as well. And if you're using one of our other packages, you'll most likely already have.

    But each package may use a different version of Skybrud.Essentials, which could then cause some problems. NuGet is generally great at handling this, but it could cause problems for the Umbraco package, as the Umbraco package format doesn't really handle dependencies at all, so the version of Skybrud.Essentials from the most recently installed package would typically win - even if it is the oldest version. I can see that this could cause some headaches, so it also serves as an argument for removing the dependency as you did your this PR.

    The latter is why I personally don't really like the Umbraco package format. So if this was a Skybrud package, I would just have kept the dependency. But while I'm the current "steward" of this package, it is a community package after all, so I'd like to hear your thoughts as well 😉

@enkelmedia
Copy link
Author

Hi!

Sorry for late reply,

Project Format

I don't have the time at the moment to change this, since it works with the current format I guess that this might be more "nice to have" than "must have" =D Might be something to look at in the future.

Assembly conflicts

Not sure either, I noticed them but they did not have any impact on the artifacts so I did not spend to much time trying to solve it.

DatePicker

That's true! Not sure why I missed that, I would have to look closer at that

Colors

Sounds fair

Skybrud.Essentials

I do see that point that your're making, when I did the rewrite I was only considering the fact that it was a small part of the Essentials-dependency that was used and figured that it would be a good idea to reduce the number of dependencies and also to avoid the issue that you're describing with different versions of the DLL - basically making it a "own working unit", also making the code-base "complete" in the way that there is no external thing that out of control for the package that is used. But, as you say, it's a potential nightmare having to fix a but in 10 different packages over just updating the dependency.

If you would prefer keeping the dependency on Skybrud.Essentials I could just add it back again? =D

// m

@stefankip
Copy link

Too bad this PR sort of 'died'.

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.

4 participants