-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Committing code to 'tag' field type. #716
Committing code to 'tag' field type. #716
Conversation
packages/framework/src/Console/Commands/MakePublicationTypeCommand.php
Outdated
Show resolved
Hide resolved
Sounds good.
Okay yeah this is definitively a good idea since the input captured is also strings. We can then cast actual integers to integers when/if we need it.
I'll take a look. The reason I added this was that when testing if the answer wasn't correct it'd get stuck in an infinite loop. Same if you ran the command with the "-n" flag.
Will do. I recently added a static analyser that will fail + add comments when a fixme is found.
Tag seems appropriate.
That makes sense.
I'm not seeing this in the diff. Where is it? |
packages/framework/src/Framework/Features/Publications/Models/PublicationFieldType.php
Outdated
Show resolved
Hide resolved
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.
Just gonna resolve the todos then I'll merge!
Mainly it includes the new tag feature, but also some other minor improvements such as:
Please look for the string FIXME in the code, I used this to mark some things which I think you should review just to make sure. It should not be anything to fix, but just to review.
The new command is called makePublicationTag ... I called it "tag" because this is the most obvious use case, but it's basically the action of chosing an item from a list, doesn't have to be tag
This command creates (or adds to) a file called tags.json ... the fact that we now have another file in the root directory IMHO strengthens the case for the argument that all content files should be under a content directory
Also, fom the above paragraph you will deduce that tags are global. Yes they are ... initially I had a version which did both global and per-publication tags but the code got messy and there are some edge cases which appear which you deal with this configuration ... so the global version is much simpler (code wise) so this is what I opted for. I think this is OK because for most applications, you wont have like 50 tags, probably more like 5-10 max so this is OK
Give it a try and let me know what you think
Oh yes, I also added a check to make sure you can't add the same filename twice when defining a publication type