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

Support for Formatting groups #7

Merged
merged 23 commits into from
Oct 7, 2023
Merged

Support for Formatting groups #7

merged 23 commits into from
Oct 7, 2023

Conversation

liprec
Copy link
Contributor

@liprec liprec commented Nov 1, 2022

As the current version of the powerbi-visuals-utils-formattingmodel does not support Formatting groups and to em that is one of the big improvements of the new formatting pane, I extended the current module to include the support for Formatting group.

Changelog

  • Added support for creating custom Formatting groups
  • Added extra attributes to Card, like missing topLevelToggle and disabled
  • Add extra (optional) visible slice attribute for easier show/hide slices in a dynamic way
  • Synced license in package.json to repo license

Please take the option to review this PR and I am open to discuss anything, like naming, structure etc.

Several points to discuss:

  • class naming, like Group
  • use/rename getFormattingGroup and getFormattingCard
  • change version

@enema17484
Copy link

Would be great if this can be reviewed by the mandatory reviewers as it's a blocker to a lot of people's work

src/FormattingSettingsComponents.ts Outdated Show resolved Hide resolved
src/FormattingSettingsComponents.ts Outdated Show resolved Hide resolved
src/FormattingSettingsComponents.ts Outdated Show resolved Hide resolved
src/FormattingSettingsComponents.ts Outdated Show resolved Hide resolved
src/FormattingSettingsComponents.ts Outdated Show resolved Hide resolved
src/FormattingSettingsComponents.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
@liprec
Copy link
Contributor Author

liprec commented Feb 28, 2023

@ShereinDabbah I have updated the PR based on your comments, but three remarks are still open as I have questions for you.

@liprec liprec requested review from ShereinDabbah and removed request for AleksSavelev March 2, 2023 19:20
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
@liprec
Copy link
Contributor Author

liprec commented Mar 24, 2023

@ShereinDabbah @AleksSavelev I have updated the PR based on the comments, but I still need to confirmation if it is OK to redefining the Card object and break any backwards compatibility? (see: #7 (comment))

@liprec
Copy link
Contributor Author

liprec commented Jul 7, 2023

@ShereinDabbah @AleksSavelev Any news on this PR?

@reyemb
Copy link

reyemb commented Jul 11, 2023

Thank you for your efforts and the time you've invested in putting this together. I've devoted considerable hours to this topic, and I must express some disappointment. When people are encouraged to participate, it can be disheartening when feedback is not forthcoming. The exchange of thoughts and comments are what keeps a community vibrant, so it's surprising to find that aspect lacking in this case.

@AleksSavelev AleksSavelev requested review from AleksSavelev and ShereinDabbah and removed request for AleksSavelev July 11, 2023 10:52
Copy link
Contributor

@AleksSavelev AleksSavelev 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 suggestions from my side, but we should also wait for a review from @ShereinDabbah.

src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
src/FormattingSettingsComponents.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
src/FormattingSettingsService.ts Outdated Show resolved Hide resolved
@KamranNaeem
Copy link

Will groups eventually be added to formattingModel?

@liprec
Copy link
Contributor Author

liprec commented Aug 17, 2023

@ShereinDabbah any news on the review of the PR 'Support for Formatting groups'?

@pochuanchan
Copy link

I also need this feature to develop my custom visual. Thank @liprec for the contribution and the PR. @ShereinDabbah could you please review it?
I have been waiting for this feature for a while. It is frustrating that powerbi-visuals-utils-formattingmodel doesn't support the formatting group as it should.

@M-Humza
Copy link

M-Humza commented Aug 29, 2023

@ShereinDabbah Please do your job and review this. Assign this to another reviewer if you don't have the time. You have someone like @liprec doing awesome work and it's just sitting there for months. Groups should have been implemented in the formatting models in the first place.

@reyemb
Copy link

reyemb commented Sep 3, 2023

I've forked and incorporated liprec's pull request into a repository. To get started, you can install the package using:

npm install powerbi-visuals-utils-formattingmodel-community

Find the repository here:
GitHub Repository

This version allows the use of formattingGroups. A simple example to illustrate how to implement this feature is available here:

Demo Repository

For implementation details, refer to the src/settings.ts file in the demo repository.

A big shout-out to liprec

@Vivaldi4S
Copy link

@reyemb You are a life saver. Thank you so much! Also, thank you @liprec for all your awesome work

Copy link
Contributor

@ShereinDabbah ShereinDabbah left a comment

Choose a reason for hiding this comment

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

Thanks @liprec , I approved the PR after all comments were fixed.
@AleksSavelev Please can you check last comment from @liprec

@AleksSavelev
Copy link
Contributor

@liprec
Please, rebase your branch and run npm audit fix to fix audit warnings.

And I'm ready to merge and publish it. Thank you a lot for your patience and cooperation!

@liprec
Copy link
Contributor Author

liprec commented Oct 3, 2023

@AleksSavelev I will rebase and solve the npm audit issues, but how do we handle the open backward compatibility comment: #7 (comment)

Keep the export type Cards = SimpleCard | CompositeCard; ?

@liprec
Copy link
Contributor Author

liprec commented Oct 6, 2023

Should be good to go @AleksSavelev and @ShereinDabbah 🚀

@AleksSavelev AleksSavelev merged commit 599f78f into microsoft:main Oct 7, 2023
1 check passed
@AleksSavelev
Copy link
Contributor

Thanks to everyone, especially @liprec! I'll publish it in the upcoming few days

@M-Humza
Copy link

M-Humza commented Oct 17, 2023

Thanks to everyone, especially @liprec! I'll publish it in the upcoming few days

Will this community version of the formattingmodel be viable for publishing the visual?

@AleksSavelev
Copy link
Contributor

AleksSavelev commented Oct 18, 2023

Thanks to everyone, especially @liprec! I'll publish it in the upcoming few days

Will this community version of the formattingmodel be viable for publishing the visual?

A new version of powerbi-visuals-utils-formattingmode@6.0.0l is published. It includes all the changes from this PR.

Is there any reason why you want to use the community version instead?

@M-Humza
Copy link

M-Humza commented Oct 18, 2023

Thanks to everyone, especially @liprec! I'll publish it in the upcoming few days

Will this community version of the formattingmodel be viable for publishing the visual?

A new version of powerbi-visuals-utils-formattingmode@6.0.0l is published. It includes all the changes from this PR.

Is there any reason why you want to use the community version instead?

I didn't know that was published. Thank you for informing me. I appreciate all your efforts and your time!

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.