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.

Sign up for GitHub

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

Jump to bottom

feat(Pagination): replace toggleTemplateProps with paginationToggleTe… #201

Merged
merged 1 commit into from
Jan 9, 2023
Merged

feat(Pagination): replace toggleTemplateProps with paginationToggleTe… #201

merged 1 commit into from
Jan 9, 2023

Conversation

gitdallas
Copy link
Contributor

closes: #147

thatblindgeye
thatblindgeye approved these changes Jan 9, 2023
View reviewed changes
Copy link
Collaborator

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Looks good! Only nit is using rename vs remove, but not a blocker.

@gitdallas
Copy link
Contributor Author

Looks good! Only nit is using rename vs remove, but not a blocker.

Gonna blame @wise-king-sullyman's generator 😜

@wise-king-sullyman
Copy link
Collaborator

I mean, the old prop was removed was it not 😂

Really though I'd be on board with standardizing on calling a rename mod a rename, and an actual removal a remove, it just seemed that we were just naming the rules remove for either case so far from what I could see so I went with that.

@gitdallas
Copy link
Contributor Author

@wise-king-sullyman I suppose the generator could check if the replacement is empty str to determine and do both ways

@wise-king-sullyman
Copy link
Collaborator

wise-king-sullyman commented Jan 9, 2023 •
edited
Loading

Yeah I'm still planning to circle back to the generator when I get a free moment, restructure things a bit and add a few other common use cases like that in a more straightforward way.

That would definitely be a quicker way to implement replace and remove functionality though. I might throw up a quick PR for that.

@gitdallas
Copy link
Contributor Author

@wise-king-sullyman but can you pr review? 😅

wise-king-sullyman
wise-king-sullyman approved these changes Jan 9, 2023
View reviewed changes
Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

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

Yes

generate-replace-prop-v5.js
@@ -9,7 +9,7 @@ const lowerCasedComponentName = componentName[0].toLowerCase() + componentName.s
const newRuleName = `${lowerCasedComponentName}-remove-${oldPropName}`

if (!componentName || !oldPropName || !newPropName || !referencePR) {
console.log('usage: node generate [componentName] [oldPropName] [newPropName] [referencePR]');
console.log('usage: node generate-replace-prop-v5 [componentName] [oldPropName] [newPropName] [referencePR]');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoops, nice catch.

@wise-king-sullyman wise-king-sullyman merged commit 08126b5 into patternfly:main Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wise-king-sullyman wise-king-sullyman wise-king-sullyman approved these changes

@thatblindgeye thatblindgeye thatblindgeye approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pagination - renamed prop
3 participants
@gitdallas @wise-king-sullyman @thatblindgeye