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

Make renderer and utils function publicly available (i.e., remove @internal markers) #204

Merged
merged 2 commits into from
Nov 22, 2019

Conversation

lehnerchristian
Copy link

@lehnerchristian lehnerchristian commented Nov 14, 2019

closes #203

prerequisites:

  • branch is up-to-date with the branch to be merged with, i.e. develop
  • build is successful
  • code is cleaned up and formatted
  • tested with Firefox 52, Firefox 57+, Chrome 64+, MS Edge 16+

Summary

  • Remove @internal marker from Renderer, which makes them publicly available in the .d.ts files

@lehnerchristian lehnerchristian marked this pull request as ready for review November 14, 2019 15:06
@thinkh thinkh changed the title remove @internal markers. closes #203 Make renderer publicly available (i.e., remove @internal markers) Nov 15, 2019
Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

I'm fine with the changes.

@puehringer
Copy link
Contributor

@thinkh @lehnerchristian @sgratzl would you mind also removing the @internal from the dialogs in ui/dialogs/?. This would make it much easier to extend/modify the behavior of the toolbar actions. For example, checking the Filter rows containing missing values when filtering categories by default.

@lehnerchristian
Copy link
Author

would be fine for me. @thinkh , what would you say?

@thinkh
Copy link
Member

thinkh commented Nov 21, 2019

@dvmichaelpuehringer I'm fine with this change. Could you please open another issue/PR for this?

@lehnerchristian Could you please file an additional PR for the LineUp 4 then?

@thinkh thinkh added lineup: v3 All issues related to LineUp v3 release: minor PR merge results in a new minor version labels Nov 21, 2019
@sgratzl
Copy link
Member

sgratzl commented Nov 21, 2019

re internal: ok

For example, checking the Filter rows containing missing values when filtering categories by default.

don't get it tbh, you mean when the user click on the filter icon it will automatically check this one checkbox?

@puehringer
Copy link
Contributor

@sgratzl Yes, as there is currently no way of passing a default configuration unfortunately, this would be one way of checking it by default. I.e. when a filter is applied, filter the missing values if it was not unchecked.

@lehnerchristian
Copy link
Author

@dvmichaelpuehringer I'm fine with this change. Could you please open another issue/PR for this?

@lehnerchristian Could you please file an additional PR for the LineUp 4 then?

sure!

@lehnerchristian
Copy link
Author

@thinkh , @sgratzl I've also removed the @internal markers from the utils.ts file as they may also be interesting for when subclassing from Renderers. so please check my latest commit again and merge if it's ok for you

Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

Fine for me.

@thinkh thinkh changed the title Make renderer publicly available (i.e., remove @internal markers) Make renderer and utils function publicly available (i.e., remove @internal markers) Nov 22, 2019
@thinkh thinkh merged commit 712fe66 into develop Nov 22, 2019
@thinkh thinkh deleted the clehner/203_remove_internal_from_renderers branch November 22, 2019 11:22
@thinkh thinkh mentioned this pull request Jan 16, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lineup: v3 All issues related to LineUp v3 release: minor PR merge results in a new minor version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants