Skip to content

Solves #37 - Support for Umbraco 8 - #38

Open
enkelmedia wants to merge 5 commits into
umbraco-community:masterfrom
Obviuse:37
Open

Solves #37 - Support for Umbraco 8#38
enkelmedia wants to merge 5 commits into
umbraco-community:masterfrom
Obviuse:37

Conversation

@enkelmedia

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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