Review of 1.0.0 Beta?

[leekelleher]

Hi @patrickdemooij9, I've given the 1.0.0-beta a trial run, and kept notes of bugs, observations and suggestions, would you like me to share them with you? I didn't want to bombard you with unsolicited feedback.

re: bugs - you've already spotted and fixed some of the bugs since the beta was released, specifically c896796 and 7c0edca.

Forewarning that my notes and suggestions will be very subjective, so you can ignore them, or take them with a pinch of salt. 😬

[patrickdemooij9]

Hi @leekelleher. That is just amazing to hear! I would love to know all your bugs, observations, and suggestions. Looking at the suggestions would also be a great way to see how other people would use the package as I was mostly programming it from my experiences.

Just drop them in the conversation here and then I can make separate issues for them where needed.

[leekelleher]

Review of uSeoToolkit v1.0.0-beta

Keep in mind that all comments, suggestions below come from a place of wanting you and the package to do well. Most of them are things that come from me having developed Umbraco packages for 15 years, and working with many client CMS editors and their understandings.

Above all, you've done a sterling job, great work getting this far! 🌟

Naming

• I'm not keen on the "u" prefix, it feels dated, but that's mostly because I've seen enough "u" prefixed packaged to make me bork. 🤮

• I think SEO Toolkit is a stronger product name, or more verbose SEO Toolkit for Umbraco, (if it needs clarify outside of Umbraco ecosystem, e.g. on the NuGet package). It does what says on the tin.

• The SEO acronym should be consistent across the package, in uppercase. "SEO" not "seo" or "Seo".

Document Type Configuration

• The Context App label "Seo Settings" could be reduced to "SEO"?

• The "Enable Seo Settings" toggle field takes up a lot of whitespace. I'm not sure what an alternative suggestion is yet, just flagging that it feels like much wasted space.

• The descriptions for the SEO settings fields seem forced and mostly a verbose repetition of the field's label, e.g. "Open Graph Description" label has a description of "Description for Open Graph". There could be an opportunity to explain how the meta-data and Open Graph fields are used, etc. But then venturing into educating the user/developer (whoever is configuring this?)

• 🐛 Open Graph Image settings only lists "Page Name" in the dropdown. Will it work with Umbraco.MediaPicker3? Ignore this, appears to be fixed: 7c0edca.

• The UI of the "Open Settings" button doesn't feel "Umbraco-esque". It could potentially use a similar UI as MNTP, where the initial button says "Add field", that would open a list of property field name, once selected they'd appear in a list of , which would then be sortable. It would remove the need for an side panel to remove or sort the fields?

• "Seo Settings saved!" success notification. It feels unnecessary to have a double notification on save. As the user would consider the SEO settings are part of the Document Type, so would only need a single notification.

Content App

• Flagging acronym consistency, "SEO" not "Seo".

• I'm wondering if this should appear before the Info app?

• The "Edit" button - wondering if this could be placed elsewhere? Adds much whitespace to the top of the app. Maybe could be placed in the group/panel/box header?

• The "Canonical Url" link opens in same tab/window - could open in new tab? (with all the noopener attributes, etc.)

• The workflow of the "Finish" button doesn't feel natural (to me). Having to press a button above the fields feels like I'm moving in the wrong direction. I'm unsure what to suggest, I understand the technical reason for needing a "Finish" button, just flagging it.

• On a side note, @mattbrailsford did find a way to get the Save/Publish buttons to appear on a Content App. Remind me to find out what the code is for this. 🤔

• 🐛 If you press the "Edit" button, make some edits, then switch to the "Content" app, then back to "SEO" app, the fields are all blank - and you can't press the "Finish" button. There is a JS error: Cannot read properties of undefined (reading 'alias'), but I haven't investigated the cause of this.

• Unsaved content amends - on the "SEO" app, there's a big yellow alert "Make sure to save/publish your changes to see the updated values here!". The colour choice may need reviewing, I found it difficult to read. Umbraco has styles for various alert boxes.

• Spotted a console.log left in there - from seoSettings.controller.js line 84.

SEO Toolkit Section

• I couldn't tell from the documentation, is uSeoToolkit.Umbraco.RobotsTxt a separate install from uSeoToolkit.Umbraco? I installed it anyway.

• The same with uSeoToolkit.Umbraco.Sitemap, unsure whether it was already installed or I had to add it manually - I installed it anyway. Although I didn't see any "XML Sitemap" menu/configuration in the SEO Toolkit section.

• The "SEO Toolkit" section didn't show up. I checked the user roles/groups, but "uSeoToolkit" wasn't there. Maybe a migration didn't run? I'm not sure. At this point I manually added the user role/group, added it to my user and the "Seo Toolkit" section now showed up.

• I quite like this separate section, has a nice feel to it. 🤩

• The section label could be simplified to "SEO"? (The left-side tree title still looks good as "SEO Toolkit").

• 🐛 The links in the 4 boxes on the dashboard aren't wired-up, they all point to item.link - probably a small AngularJS expression needed, e.g. ng-href="{{item.link}}"?

• The context menu for each of the tree items throws a null reference error. Are these meant to have menu items? If not, there's a way to hide the three-dot menu button. Remind me to find the code for you.

• The License tree item has a green icon. I'm unsure why it's different.

Tag Helpers

• Manually adding stuff to _ViewImports.cshtml feels like quite a faff. This is more of a personal criticism of ASP.NET Core. Too many "just" dev tasks.

• With the <meta-fields> tag helper, I noticed that the attributes in the output are using single quotes - I appreciate that it's valid syntax, but it niggles me - I want consistent double-quote style in my markup.

• 🐛 With <render-script> tag helper, I got an error A view with the name ~/Views/ScriptManager/GTM/HeadBottom.cshtml could not be found Ignore this, appears to be fixed: c896796.

---

</review>

With the suggestions, I'm happy to send over pull-requests if you want? Just I didn't want to make any assumptions before posting.

[patrickdemooij9]

@leekelleher, Wow! Just wow, I had not anticipated so much feedback, but I am very happy with it.

I'll try to make some issues about the things that you found. I think all of them are valid findings about the package that could definitely be fixed/improved!

Will just go through some of the things and comment on them.

• I'm not keen on the "u" prefix, it feels dated, but that's mostly because I've seen enough "u" prefixed packaged to make me bork. 🤮

I think this is a fair point. I chose uSeoToolkit as I was eventually planning on getting the domain for it and then being able to host a website with that domain. But I also noticed yesterday while listening to UmbraCoffee that it just doesn't really make sense. And honestly, this is probably the only time to really do that. I'll think about it, but SeoToolkit does seem better to me.

The SEO acronym should be consistent across the package, in uppercase. "SEO" not "seo" or "Seo".

Big agree. Created a ticket for it #13

The Context App label "Seo Settings" could be reduced to "SEO"?

Does keep it in line with the context app for the content section so I think this is a good idea to go with. #14

The UI of the "Open Settings" button doesn't feel "Umbraco-esque". It could potentially use a similar UI as MNTP, where the initial button says "Add field", that would open a list of property field name, once selected they'd appear in a list of , which would then be sortable. It would remove the need for an side panel to remove or sort the fields?

Yeah, this is a difficult one. The reason I added the side panels is to make the it look cleanier to work with, but it does indeed take away from the feel of the Umbraco backoffice. I'll try make the changes to something like MNTP (Or contentment for that matter) and see if it doesn't look too cluttered. But created an issue to change/investigate some styling changes there #15

Content App

All good points (Especially the bugs). Will make a bunch of tickets for that.

I couldn't tell from the documentation, is uSeoToolkit.Umbraco.RobotsTxt a separate install from uSeoToolkit.Umbraco? I installed it anyway.
The same with uSeoToolkit.Umbraco.Sitemap, unsure whether it was already installed or I had to add it manually - I installed it anyway. Although I didn't see any "XML Sitemap" menu/configuration in the SEO Toolkit section.

That is strange. It should install those packages as part of the uSeoToolkit.Umbraco package.

Will see if I can reproduce that issue.
XML indeed does not add a node to the section as the only (current) configuration is located in the document type context app.

The "SEO Toolkit" section didn't show up. I checked the user roles/groups, but "uSeoToolkit" wasn't there. Maybe a migration didn't run? I'm not sure. At this point I manually added the user role/group, added it to my user and the "Seo Toolkit" section now showed up.

It doesn't automatically create a group for it right now. Although now that you say it, it would be nice to have a migration run to create a uSeoToolkit group that people could easily be added too. Great idea! #16

The links in the 4 boxes on the dashboard aren't wired-up, they all point to item.link - probably a small AngularJS expression needed, e.g. ng-href="{{item.link}}"?

Yep, needs fixing. Totally forgot to wire those up to the documentation pages.#17

The context menu for each of the tree items throws a null reference error. Are these meant to have menu items? If not, there's a way to hide the three-dot menu button. Remind me to find the code for you.

Reminder sent ;). They shouldn't currently need a tree item (perhaps for the sitemanager as that one has a create action) #19

The License tree item has a green icon. I'm unsure why it's different.

Idea was to make it green if license was all set up and good to go and red if there was an issue. But might just get rid of the green and only use the red if there is actually an issue (license logic still has be created).

Manually adding stuff to _ViewImports.cshtml feels like quite a faff. This is more of a personal criticism of ASP.NET Core. Too many "just" dev tasks.

I wish there was a way to automatically do this. It would make configuration soooooooooooooooooooo much easier!

With the tag helper, I noticed that the attributes in the output are using single quotes - I appreciate that it's valid syntax, but it niggles me - I want consistent double-quote style in my markup.

Sounds like an easy fix, so I'll take a look at that.

But all in all, great finds and thanks a lot for going through it! You are always more than welcome to open up a PR for any of the bugs or issues that I've created.

[leekelleher]

@patrickdemooij9 We can ignore my comments about the separate NuGet package installs for RobotsTxt and Sitemap, I think my confusion was due to the "uSeoToolkit" section/user-group not being available. I'd (wrongly) assumed that installing the RobotsTxt package would activate that section... but now I realise it was a separate issue - hopefully resolved with PR #23. 😃