fix: change element name referencing function from private to protected #2128
Conversation
|
|
Hi @codyburleson we're planning to make some potentially breaking changes to this element, see #2052. Asides from that, Perhaps in this case we could provide something like class SpicyProgressStepper extends PfeProgressStepper {
static allowedItems = ['spicy-progress-step']; // default ['pfe-progress-step']
}Would that suit your needs? |
|
P.S. if you have the time and the inclination, you're welcome to grab #2052 |
|
Hi, @bennypowers, I think that your suggestion could work, allowing something like the following:
I'm not sure the details on why you might prefer it that way over just making the private method protected, but I trust your head is much better aligned to what you're trying to achieve with the library. As we feel like our decision to follow along with Patternfly Elements is pretty firm now, we do hope to eventually be a contributing member of the community. So, we definitely want to get "in" on the goings-on. Perhaps this is a good place to summarize our strategy in short in case you have comments about how we're going about it. If you have no comments, you at least know how we're operating for future interactions. We chose to fork the repo and in our initial phase, just extend the existing components, for little more reason than to change the element tag names. On our first pass, we make no more modifications than that. This decision was so that we don't inadvertently get our corporate users accidentally using pfe elements on accident, when they should always be pointing to our corporate lib. The longer term point of view here is that with our own repo, we can then create some components and themes and such that will likely remain specific to our organization and its brands and applications. We want to update our repo from the upstream at times to leverage the benefit of your team's work and the community work. Likewise, if we make improvements that are not org specific in nature, we fully intend to contribute those improvements back. We're just getting started. I've successfully been able to extend a component in a very simple way, just to change the tag name. Here, for example, is all it appears to take (extending codeblock, to change tag name): import { customElement } from 'lit/decorators.js';
import { pfelement } from '@patternfly/pfe-core/decorators.js';
import { PfeCodeblock } from './pfe-codeblock';
@customElement('cnh-codeblock')
@pfelement()
export class CnhCodeBlock extends PfeCodeblock {}
declare global {
interface HTMLElementTagNameMap {
'cnh-codeblock': CnhCodeBlock;
}
}I am currently hung up on the bundling as I do not see how you are bundling components in your repo. When I deploy what is built with Anyway, I hope that helps you understand our strategy (and feel free to poo on it; we're all ears). When you say... if you have the time and the inclination, you're welcome to grab #2052 I assume you mean in the way you suggested? I.e....
...since my original pull request was just changing the private to protected so the method could be overriden? Or, are you suggesting, based on the linked design specification from PatternFly v4 Progress Stepper that there is more than you can use help with? Thank you for your kind consideration! We look forward to learning more and being team players if we can. |
|
Oh, wow @codyburleson what a thorough comment. I'm really glad to read all of this. A few notes:
a general trend to prefer declarative config over imperative code or oo inheritance, wherever practical
🎉
Your reasoning here is solid, and this is in fact what we ourselves are doing with Red Hat Design System.
Forking works fine, of course, but I recommend instead of forking that you establish your own repo which depends on PFE tools and packages. This is how RHDS does it. One of the chief advantages here is not having to maintain a monorepo, as well you can drastically reduce the amount of code you have to maintain. So your code example might look like this instead: import { customElement } from 'lit/decorators.js';
import { PfeCodeblock } from '@patternfly/pfe-codeblock';
@customElement('cnh-codeblock')
export class CnhCodeBlock extends PfeCodeblock {}
declare global {
interface HTMLElementTagNameMap {
'cnh-codeblock': CnhCodeBlock;
}
}Note also that I removed the
Yes, please!
Take a look at how we do it in RHDS. TL;DR: for a single minified bundle use esbuild, but if you're going to package up your components for other projects to use, I'd recommend not bundling them, and instead make bundling an app-level concern, to be solved with the usual suspects (esbuild or rollup are preferable). The exception to that rule would be transforming css files to js objects, which we do with
Yes feel free to open discussion threads here, which are ideal because they can help the next one in line.
The intention is that the next work done on progress stepper will be to overhaul the styles (and possibly APIs) to better align with PFv4. Along the way, the If that's of interest,
We're likely to want to make API changes, so be prepared for some potential back and forth in that area |
|
#2292 replaces the old progress-steps. it contains Be aware that in final release the tagnames and class names will change |
2 participants
What has changed and why
elements/pfe-progress-steps/pfe-progress-steps.tsstepItems()fromprivatetoprotectedpfe-progress-steps-itemtomy-progress-steps-item). In our case it is useful to override PFE classes and change the prefix to help ensure our corporate user base uses our customizations and does not inadvertently use the PFE elements directly. As such, any private methods that make reference to the element names should be protected. There may be more cases across the code base where this occurs, but this is the only element we're working with right now.