Skip to content
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

Fix conversion between PnPCore enum values and PnP.Framework enums #297

Merged
merged 1 commit into from
May 3, 2021

Conversation

jackpoz
Copy link
Contributor

@jackpoz jackpoz commented Apr 30, 2021

This PR fixes a conversion between PnPCore enum values and PnP.Framework enums. The old code does a C-style cast from the PnPCore enum to the PnP.Framework one but the 2 enums have the possible values defined in different order:

PnPCore:

  //
  // Summary:
  //     Alignment of the title in a page header
  public enum PageHeaderTitleAlignment
  {
      //
      // Summary:
      //     Page title is centered
      Center,
      //
      // Summary:
      //     Page title is left aligned
      Left
  }

PnP.Framework:

  public enum ClientSidePageHeaderTextAlignment
  {
      /// <summary>
      /// Align Left
      /// </summary>
      Left,
      /// <summary>
      /// Align Center
      /// </summary>
      Center,
  }

This means PageHeaderTitleAlignment.Left with value 1 gets converted to ClientSidePageHeaderTextAlignment of value 1, so ClientSidePageHeaderTextAlignment.Center .

The current code already tries to convert from an enum to another through .ToString() but parses it back to PnPCore enum instead of PnP.Framework.

To compare the old and the new code:

  • old: PnPCore enum -> ToString() -> parse into PnPCore enum -> c-style cast to PnPFramework
  • new: PnPCore enum -> ToString() -> parse into PnPFramework enum -> c-style cast to PnPFramework
    While at it I updated 2 other similar fields Type and LayoutType that never showed any issue because the enums are defined in the same order in both projects.

In addition to this it might be worth to consider having the 2 enums with the same order in both projects.

Fix #266

@jansenbe jansenbe self-assigned this May 3, 2021
@jansenbe jansenbe added area: page transformation 🦾 Page transformation issue or pull request area: provisioning ⚙ Provisioning engine issue or pull request labels May 3, 2021
@jansenbe
Copy link
Contributor

jansenbe commented May 3, 2021

Good update @jackpoz , will also update the enum in PnP.Core

@jansenbe jansenbe merged commit 170d0e2 into pnp:dev May 3, 2021
jansenbe added a commit that referenced this pull request May 3, 2021
@jackpoz jackpoz deleted the fixes/page-alignment branch December 4, 2022 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: page transformation 🦾 Page transformation issue or pull request area: provisioning ⚙ Provisioning engine issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ClientSidePage Header always have TextAlignment set to "Center"
2 participants