Skip to content

Implement Fieldmanager_Content, Plaintext, HTML, and Markdown#781

Merged
dlh01 merged 24 commits intomasterfrom
feature/FLAME-113/content-field
Feb 12, 2021
Merged

Implement Fieldmanager_Content, Plaintext, HTML, and Markdown#781
dlh01 merged 24 commits intomasterfrom
feature/FLAME-113/content-field

Conversation

@jameswburke
Copy link
Copy Markdown
Contributor

@jameswburke jameswburke commented Oct 20, 2020

Summary

Implements an abstract Fieldmanager_Content class, and 3 implementations that supports rendering static text (Fieldmanager_Content_Plaintext), html (Fieldmanager_Content_Markup), and markdown (Fieldmanager_Content_Markdown).

Notes for reviewers

Questions for reviewer,

  • Should Parsedown() be included differently? Perhaps composer, or a different directory?
  • Any tips for how to better structure or use existing Field functionality?
  • Any thoughts on additional tests to implement?

Changelog entries

  • Added: Fieldmanager_Content abstract class.
  • Added: Fieldmanager_Content_Plaintext, Fieldmanager_Content_Markup, and Fieldmanager_Content_Markdown classes and unit tests.

Ticket

https://alleyinteractive.atlassian.net/browse/FLAME-113

@jameswburke jameswburke linked an issue Oct 20, 2020 that may be closed by this pull request
@dlh01
Copy link
Copy Markdown
Member

dlh01 commented Oct 21, 2020

Some drive-by comments...

  • The name Fieldmanager_Content potentially implies some sort of relationship to post_content that might prove confusing. I don't have a great alternative to offer, though. Fieldmanager_Display? Fieldmanager_InBetween?

  • I think it would be great if the field could accept the path to a file as an alternative to accepting the content string, as it would allow a developer to maintain a separate .md file rather than have to manage the content within PHP variables.

  • Along the lines of Matt's suggestion: There's currently one class responsible for handling three content types, but I wonder whether the developer experience would be improved by instead having three classes, one for each content type. For maintainers, these classes would likely be smaller and thus have less to maintain. For developers, only one thing needs to be specified to use the field (the class) instead of two (the class and the type property). Lastly, FM already has abstract classes for similar situations (e.g. Fieldmanager_Options).

@jameswburke
Copy link
Copy Markdown
Contributor Author

@dlh01, thanks David, I've split out Fieldmanager_Content into Fieldmanager_Plaintext, Fieldmanager_Markup, and Fieldmanager_Markdown. I had originally gone down this path, but found it was possibly too verbose, but your suggestion encouraged me to revisit. I do think it's a better developer experience at the end of the day, even at the expense of some additional code to maintain.

I like your point about loading files, but I think file_get_contents is a simple way to achieve that. I wouldn't add support for anything more complex as part of this initial implementation.

new Fieldmanager_Markdown(
	[
		'content' => file_get_contents( __DIR__ . '/readme.md' ),
	]
);

@mboynes
Copy link
Copy Markdown
Contributor

mboynes commented Oct 21, 2020

@jameswburke I like this new direction. I think it would be beneficial for the three classes to share an abstract class. An abstract class would DRY up the code a bit and make it easier for a third party to add their own variant, e.g. if someone wanted to add their own Textile field or whatever.

@dlh01
Copy link
Copy Markdown
Member

dlh01 commented Oct 21, 2020

file_get_contents() certainly works, and it isn't mutually exclusive with adding a file property later. The downside to consider is performance: The file has to be read from disk immediately rather than only in the moment that Fieldmanager needs it.

@jameswburke
Copy link
Copy Markdown
Contributor Author

Updated autoload approach to support a Fieldmanager\Library\Package\Class format, and implemented abstract class Fieldmanager_Content and refactored for other content fields.

@jameswburke jameswburke changed the title Implement Fieldmanager_Content field Implement Fieldmanager_Content, Plaintext, Markup, and Markdown Oct 21, 2020
@jameswburke
Copy link
Copy Markdown
Contributor Author

@mboynes & @dlh01, could you take another look at my latest pass on this? Thanks!

Copy link
Copy Markdown
Contributor

@mboynes mboynes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more changes then it looks good to me to ship!

jameswburke and others added 3 commits October 23, 2020 12:22
Co-authored-by: Matthew Boynes <mboynes+git@alley.co>
Co-authored-by: Matthew Boynes <mboynes+git@alley.co>
Co-authored-by: Matthew Boynes <mboynes+git@alley.co>
jameswburke and others added 3 commits October 23, 2020 12:22
Co-authored-by: Matthew Boynes <mboynes+git@alley.co>
…leyinteractive/wordpress-fieldmanager into feature/FLAME-113/content-field
Copy link
Copy Markdown
Member

@dlh01 dlh01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, too. I've noted a few documentation issues to tidy up, along with a couple of other suggestions.

I'm still a little hesitant about some of the class naming, although I think we can still merge this and revisit before release.

Fieldmanager_Markup is ambiguous to me; Matt gave the example of a child class that renders Textile, and Textile is also a markup language.

Also, leaving out the _Content_ prefix from the child class names is potentially cutting us off from the obvious name of a future field that is designed to actually save Markdown, or whatever. The context classes are Fieldmanager_Context_*; the datasource classes are Fieldmanager_Datasource_*; maybe these should be Fieldmanager_Content_*.

@jameswburke jameswburke changed the title Implement Fieldmanager_Content, Plaintext, Markup, and Markdown Implement Fieldmanager_Content, Plaintext, HTML, and Markdown Feb 12, 2021
@dlh01 dlh01 merged commit 13de756 into master Feb 12, 2021
@dlh01 dlh01 deleted the feature/FLAME-113/content-field branch February 12, 2021 17:01
@dlh01 dlh01 added this to the 1.4.0 milestone Jan 6, 2022
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.

Adding titles

3 participants