-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(segment): add umbraco segment to display modern or legacy version #4309
feat(segment): add umbraco segment to display modern or legacy version #4309
Conversation
@JanDeDobbeleer I have not written tests for this yet, as I suspect you will have some initial feedback and suggestions for me to do some rework on. |
@warrenbuckley I'n having surgery the day after tomorrow (and my GF's birthday tomorrow) so I'll probably only review somewhere next week when I'm feeling a bit recovered. |
@JanDeDobbeleer I hope all goes OK for you & your GF has a good birthday, as it's mine tomorrow too 🎉🍰 |
@JanDeDobbeleer friendly bump 👋 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@warrenbuckley I have a proposal. As you're the first to have the need to match on case insensitive filenames, and the work needed to resolve this correctly is pretty impactful (I tried to have a go at it, but it's a lot of work), I have the proposal to use the env.LsDir('path')
functionality here and match the filename in the segment itself. You are looping them already, this would allow to match and parse when hit.
Sounds like a plan. I am away on a short break for a few days, but just leaving myself a note here to rework this and not depend on case insensitivity to work in
Question for you though @JanDeDobbeleer are segments like this designed to be able to continue working if we navigate deeper into folders and look up through parent folders ? |
@JanDeDobbeleer done the suggested refactor and let me know what you think please. If you are happy then it would be good if you can give me a pointer on tests please. Hopefully I am getting closer to finishing up for Hacktoberfest 😂 |
fc05634
to
042d104
Compare
when it navigates up folders it will change to \\workspace and \\
Thanks for the review @JanDeDobbeleer 👍 |
uses len(string) == 0 as opposed to "" for checking empty value Co-authored-by: Jan De Dobbeleer <[email protected]>
…nd return an error
…ase in the xml content
OK @JanDeDobbeleer I think I have covered all the changes and the tests are still passing, let me know what you think |
@warrenbuckley I unresolved one remark to have that final conversation. I can also make that change if agreed, I have this ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found one typo, that's all. Let's fix that and merge this!
@JanDeDobbeleer I have implemented the change that you suggested, along with fixing up the tests and yes it's loads better and simpler, so thanks 💪😍 Just super excited to try and get this across the finish line. |
@JanDeDobbeleer fixed that typo - so should all be good to go 🤞 |
@all-contributors please add @warrenbuckley for design,doc |
I've put up a pull request to add @warrenbuckley! 🎉 |
Hi @JanDeDobbeleer |
Prerequisites
Example
Example Segment configuration
Logic
The Umbraco segment is shown when
umbraco
PackageReference
umbraco.core.configurationstatus
Sample files
MyProject.csproj
web.config