Feature/heading styles #5736
Feature/heading styles #5736
Conversation
* [Shared] Add heading style support * [UWP] Add Heading Styling support
* [Android] Text Heading styles * Update samples * PR feedback * Update tests
| { | ||
| return defaultEnumValue; | ||
| } | ||
| return std::nullopt; |
There was a problem hiding this comment.
@RebeccaAnne,
Returnnig nullopt for empty value seems to be inconsistent with returnning the default value from the line 157 when the given string value doesn't map to enum values. For example, for "", will return nullopt, but "Hello World" for horizontal alignment will return HorizontalAlignmentLeft. returnning nullopt makes more sense since we have to know if the user set the value or not. But then the renderers have to know what is the proper default values. We could have a util function for returnning the default values for cross platforms.
There was a problem hiding this comment.
You're first point is a good one, and I updated the enum magic code to not set the default from something like "Hello World". Since most of the callers of this are coming through ParseUtil::GetEnumValue those callers will set the default anyway, but for the ones coming directly through ParseUtil::GetOptionalEnumValue should now get nullopt in that case.
It is correct that with this setup the renderers do need to know the defaults. I'm not opposed to setting something up in shared model to ensure we've got consistent defaults cross platform. Could you log an issue to track adding that?
There was a problem hiding this comment.
+1 to this, would love to move the defaults for all optionals back into the object model
* [iOS] Added heading styles * [iOS] Added heading styles * [iOS] fixed font family error & adjusted font weight value & fixed isSubtle
ghost
added
the
|
Hi @RebeccaAnne. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along. |
|
Hi @RebeccaAnne; Thanks for taking action on your previously stale pull request. Resetting staleness. |
* [Shared] Add heading style support (microsoft#5702) * [UWP] Add Heading Styling support (microsoft#5708) * [Shared] Add heading style support * [UWP] Add Heading Styling support * [Android] Add heading style support (microsoft#5740) * [Android] Text Heading styles * Update samples * PR feedback * Update tests * [iOS] Text Styles Changes (microsoft#5759) * [iOS] Added heading styles * [iOS] Added heading styles * [iOS] fixed font family error & adjusted font weight value & fixed isSubtle * String to enum handling doesn't set default. * Fix shared model tests from enum changes * small swig fix * Fix shared model break * Re-SWIG after merge * Re-SWIG Co-authored-by: Risheek Rajolu <risheekrr@gmail.com> Co-authored-by: Joseph Woo <Joseph.Woo@microsoft.com>
* [Shared] Add heading style support (microsoft#5702) * [UWP] Add Heading Styling support (microsoft#5708) * [Shared] Add heading style support * [UWP] Add Heading Styling support * [Android] Add heading style support (microsoft#5740) * [Android] Text Heading styles * Update samples * PR feedback * Update tests * [iOS] Text Styles Changes (microsoft#5759) * [iOS] Added heading styles * [iOS] Added heading styles * [iOS] fixed font family error & adjusted font weight value & fixed isSubtle * String to enum handling doesn't set default. * Fix shared model tests from enum changes * small swig fix * Fix shared model break * Re-SWIG after merge * Re-SWIG Co-authored-by: Risheek Rajolu <risheekrr@gmail.com> Co-authored-by: Joseph Woo <Joseph.Woo@microsoft.com>
Related Issue
Merges feature branch that will contain fixes for #5632, #5633, #5634, and #5631. This branch is not currently ready to merge as not all fixes are in yet.
Description
Feature branch for shared model platforms
Microsoft Reviewers: Open in CodeFlow