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

[READY] feat: add type parameters to class name #3488

Open
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

deleonio
Copy link

@deleonio deleonio commented Jul 25, 2022

TODO's

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Unit tests (npm test) were run locally and passed
  • E2E Tests (npm run test.karma.prod) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

GitHub Issue Number: #2895

What is the new behavior?

  • We can use type parameters on component classes.
@Component({
       tag: 'kol-table',
        ...
})
export class KolTable<T> implements ... {
       ...
       @Prop() public _data!: T[];
       ...

Does this introduce a breaking change?

  • Yes
  • No

Testing

Other information

@deleonio deleonio requested a review from a team July 25, 2022 07:37
@deleonio deleonio changed the title feat: add type parameters to class name [WIP] feat: add type parameters to class name Jul 25, 2022
@deleonio deleonio changed the title [WIP] feat: add type parameters to class name [READY] feat: add type parameters to class name Jul 25, 2022
@deleonio deleonio changed the title [READY] feat: add type parameters to class name [WIP] feat: add type parameters to class name Jul 25, 2022
@deleonio
Copy link
Author

deleonio commented Jul 25, 2022

Hello @rwaskiewicz,

the browserstack test failing with 30s timeout. Is that a code issue or a workload thing?

Thx

@deleonio deleonio closed this Jul 26, 2022
@deleonio deleonio force-pushed the feat/add-type-parameters-to-class-name branch from cf90f45 to 22d9858 Compare July 26, 2022 05:04
feat: add type parameters to class name
@deleonio deleonio reopened this Jul 26, 2022
@rwaskiewicz
Copy link
Member

@deleonio it appears they're running now...maybe a result of closing/opening the PR? 🤔

@deleonio deleonio changed the title [WIP] feat: add type parameters to class name [READY] feat: add type parameters to class name Jul 26, 2022
@deleonio
Copy link
Author

deleonio commented Jul 26, 2022

Hi @rwaskiewicz,

browserstack was a little bit random - but now everything looks fine.

I set the PR to ready - I hope it is okay.

My next PRs:

@deleonio deleonio force-pushed the feat/add-type-parameters-to-class-name branch from e84f6ac to 008d1dd Compare August 1, 2022 01:41
@rwaskiewicz
Copy link
Member

Thanks @deleonio! For a bigger feature like this, I'm going to label this for the team to discuss and review!

@deleonio
Copy link
Author

Hello together,

this feature is very important for us and it would be nice if you take a look at it.

Thanks a lot

@rwaskiewicz
Copy link
Member

Hey @deleonio,

This is currently in our backlog for the team to discuss. It's a interesting feature, but one that we'd like to take the time to discuss as a team. I don't have an ETA for you at this moment, and can't quite promise when we'll have the chance to take a look.

Thanks!

@deleonio
Copy link
Author

Conflicts solved.

@jackdriscollukg
Copy link

Hey! I would also like to use this feature. Thanks for opening this PR and I hope it can get merged soon.

@deleonio
Copy link
Author

Hi @jackdriscollukg, thanks for your support!

I see that you also focused on a11y - you should follow us on our community - https://github.com/public-ui/kolibri.

You are welcome!

@deleonio
Copy link
Author

deleonio commented Mar 1, 2023

Hi @rwaskiewicz,

can you make a statement about this? Is the feature coming and if so, when?

@mhartington
Copy link
Contributor

Hey @deleonio, the team has this in the backlog and will review when they can.

You do not need to ask for an update or tag various members of the ionic org 😄

@deleonio
Copy link
Author

deleonio commented Mar 6, 2023

Hello @mhartington,

thank you very much for your reply!

I think it is possible to ask from time to time. We want to point out that maybe there is a feature that is not prioritized high enough. Also, the PR is not new, seems to be well integrated (x-resyncs with no conflicts) and you have unfortunately mostly not responded.

Basically, I have to conclude that this feature is not prioritized high enough and thus does not come into focus for editing.

The thing is, this feature vastly improves numerous typing situations while remaining simple. This is a great pity.

We've been thinking about forking Stencil for months now - but it's better to contribute to Stencil than to open special construction sites.

With kind regards
Martin

@jflynn7
Copy link

jflynn7 commented May 1, 2023

Any further update on getting this merged?

@deleonio
Copy link
Author

Hi, in our team we are talking about this feature and we think it is interesting in general. Only for Angular with its proprietary template language it is unusable.

So, what can we do?

@johnjenkins
Copy link
Contributor

@deleonio - I always thought this looked useful :)
If you're still working with Stencil - the project's policy seems much more receptive to community PRs nowadays

@deleonio
Copy link
Author

deleonio commented Dec 11, 2024

@deleonio - I always thought this looked useful :) If you're still working with Stencil - the project's policy seems much more receptive to community PRs nowadays

YES! We will use that approach for our tables and form fields ... https://github.com/public-ui/kolibri/wiki/%C3%9Cbersicht-Input%E2%80%90Komponenten

@christian-bromann
Copy link
Member

If we can get an updated version of this patch I think we can work on this to get it merged. To make this simple for me, let's make sure the code is well documented, we have user documentation for it and proper test coverage.

@deleonio
Copy link
Author

We are currently working on our major release 3 with massive new features in rendering, BEM, type management and event handling.

But 2025 ;-) ... the tiny list - https://github.com/public-ui/kolibri/wiki/Stencil-Collaboration

@christian-bromann
Copy link
Member

But 2025 ;-)

Awesome, you know how to reach me 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants