-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add Custom AutoFormats with Fallback Autoformats #372
Add Custom AutoFormats with Fallback Autoformats #372
Conversation
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.
Thank you for contributing @cotw-fabier! Your changes on the AutoFormats
looks good to me. Left you two comments. PTAL.
@@ -0,0 +1,364 @@ | |||
import 'dart:convert'; |
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.
It might be better to have an example app showcasing every feature we have instead of a separate example for each feature, and a simple example to get users started quickly. I suggest to remove this example from this PR and create another PR for a complete advanced example. WDYT?
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.
Hi @Amir-P,
This is really the main example with some additions to showcase this extra feature so it could overwrite the example app you have there already if you choose to use it. I have been taking notes with the intention to eventually help with some of the documentation and covering several use cases with Fleather and Parchment. Trying to build a few cohesive examples. This and #371 both have examples working with the same "youtube" custom block example.
However, I'm not married to any of it! Definitely take what you like and leave the rest. :)
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.
In that case let's rename this into something like example_advanced.dart
, then we can simplify current example we have in another PR by removing inline images and embeds for users to get started with basics quicker. Also I can see you've defined YoutubeEmbed
but never used it. @cotw-fabier
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.
Sounds good to me. I also realized I left the old version of the autoformats.buildWithFallback command in the example. Let me clean it up and post a new commit. Should be able to do it closer to the weekend -- need to wrap up a work project tomorrow.
@@ -38,6 +38,18 @@ class AutoFormats { | |||
]); | |||
} | |||
|
|||
/// Create a new instance of [AutoFormats] with the provided [autoFormats] + fallback [AutoFormats]. |
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.
Could you please make current auto format implementations (_AutoTextDirection
, _MarkdownInlineShortcuts
, _MarkdownLineShortcuts
) public so that users have the option of choosing from?
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.
Ok, Took a second swing here. Removed buildWithFallback and just extended the fallback method. I added a public enum and some params (autoFormats and defaultAutoFormats) so you can control how fallback builds the AutoFormats class. It will gracefully fall back to its original behavior if no params are provided, or if you just add your own autoformats it'll add the fallback autoformats automatically. But now you can also build a list of desired fallback formats and it will allow you to pick and choose as desired.
I left the merge function I originally had in there because it seems ergonomic for future use but its unused at this point so it could be removed.
This is the commit with my changes: 41e72fc
Then I realized I had left a spread in there from an earlier rendition of my changes so I made a second commit to just strip that out: bf1fb43
Happy to make further changes as needed.
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.
Thank you for the changes. I'm just wondering wouldn't it be easier to just make current auto formats public (by removing the underscore in class name) instead of introducing enums and then mapping them to actual auto formats? Then a simple change on AutoFormats.fallback
would solve our issue. Something like this:
factory AutoFormats.fallback([List<AutoFormat>? additionalFormats]) {
return AutoFormats(autoFormats: [
const AutoFormatLinks(),
const MarkdownInlineShortcuts(),
const MarkdownLineShortcuts(),
const AutoTextDirection(),
...?additionalFormats,
]);
}
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.
Oh.... Thats fine with me. I would have to track down how they became private to begin with. I didn't write that bit. I'll check it out soon.
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.
They were made private from the first commit they were added. Removing the underscore in their names would make them public.
Remove AutoFormats.buildWithFallback. Add AutoFormatType enum. Extend AutoFormats.fallback() to include params to add custom formats and alter default fallback formats.
Remove unneeded spread operator.
Hey @cotw-fabier. Are you still interested in moving this forward? |
Amir, Sorry this fell down my list a bit but I am still quite interested. I took a second to look at this and it seem to make the change you're asking for I would have to rename the functions being called along with changing their reference. I think this is probably fine, but wanted to confirm? I'm going to be out of town for a few days but will revisit next week when I return. |
I'm not sure which functions you're talking about. What we discussed was to make the current auto formats public so that users can use a subset of them to instantiate |
Hi Guys,
One more PR here based on #369. This introduces a simple factory to the AutoFormat class which allows us to build it with the fallback. To keep things simple I opted to initalize the fallback and then merge that with my new list of AutoFormats. I know this isn't the most efficient but I didn't know if you wanted to leave the fallback() factory alone. Another option would have been to introduce a nullable variable so we could just add them there which would also be pretty ergonomic.
If you prefer that I could roll that in and update the PR here.
I created a new example file with autoformats and also a sample for rendering based on the issue mentioned above. Simple, but should be advanced enough so people can understand the API without too much trouble!
Thanks for the awesome package!