fix: change element name referencing function from private to protected

[codyburleson]

What has changed and why

• File: elements/pfe-progress-steps/pfe-progress-steps.ts
• Change: function stepItems() from private to protected
• Why: The private function uses a DOM querySelector to reference a PFE child element by name. Because the method is private, it cannot be overridden by a classes that extend PfeProgressSteps with the intention of changing element prefixes (e.g. pfe-progress-steps-item to my-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.

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: b2cdd257d58e64b1e2d04e2dbfccbebc9f01cf5c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[bennypowers]

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?

[bennypowers]

P.S. if you have the time and the inclination, you're welcome to grab #2052

[codyburleson]

Hi, @bennypowers,

I think that your suggestion could work, allowing something like the following:

static allowedItems = ['spicy-progress-step']; // default ['pfe-progress-step']

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 npm run build, those components still make reference to other things in node_modules. I'm curious how you bundle for deployment to nmp so that we only need to reference the single js file and can then use the element. Any tips are appreciated, but I will continue wrestling with it.

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 https://github.com/patternfly/patternfly-elements/issues/2052

I assume you mean in the way you suggested? I.e....

static allowedItems = ['spicy-progress-step']; // default ['pfe-progress-step']

...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.

[bennypowers]

Oh, wow @codyburleson what a thorough comment. I'm really glad to read all of this. A few notes:

why you might prefer it that way

a general trend to prefer declarative config over imperative code or oo inheritance, wherever practical

our decision to follow along with Patternfly Elements is pretty firm now [...] to be a contributing member of the community

:tada:

We chose to [...] extend the existing components, for little more reason than to change the element tag names.

Your reasoning here is solid, and this is in fact what we ourselves are doing with Red Hat Design System.

fork the repo

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 @pfelement decorator here, as it adds the PFElement class and attribute to all elements it decorates, and I imagine you don't need that. It will eventually be deprecated.

contribute those improvements back

Yes, please!

I am currently hung up on the bundling as I do not see how you are bundling components in your repo

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 ttsc.

Any tips are appreciated

Yes feel free to open discussion threads here, which are ideal because they can help the next one in line.

When you say...

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 static allowedItems config should be implemented, to support your case. My invitation was for your team to take that whole task on, if that fits with your capacity/inclination.

If that's of interest,

1. assign yourself the issue (or barring that, comment to that effect in the issue thread)
2. make a new development branch (you can use the github web ui for that if you want) or you can reset this branch to main and continue here
3. open up the linked design spec while working, and use the browser dev tools to derive styles from the compiled react components on that page (or read the source i guess)
4. remove all the sass features from the style scss file
5. following the conventions established in e.g. pfe-button, use the patternfly css variables to style the component per the specs from step (3)
6. when you're ready for review tag myself and a member of the design team for review

We're likely to want to make API changes, so be prepared for some potential back and forth in that area