Skip to content

Conversation

@castastrophe
Copy link
Contributor

@castastrophe castastrophe commented Mar 30, 2020

What has changed and why

Summarize files edited as part of this MR along with a brief description of what was changed/why.

  • Added a warning to be thrown before upgrade if the tag contains the on attribute
  • Updated documentation and tests, removed current examples of manually pushing the on attribute inside the pfe-content-set

Testing instructions

Be sure to include detailed instructions on how your update can be tested by another developer.

  1. Check out the branch.
  2. Edit the pfe-content-set demo page to add the on="light" attribute on the pfe-content-set and pfe-cta tags. Add an ID to one of the tags.
  3. Run npm install && npm run start pfe-accordion.
  4. Preview the accordion's demo page.
  5. Check that the console is registering 2 warnings and that the one for the tag with an ID lists the ID in the warning in square brackets:
pfe-content-set[#foo]: The "on" attribute is protected and should not be manually added to a component. The base class will manage this value for you on upgrade.
pfe-cta: The "on" attribute is protected and should not be manually added to a component. The base class will manage this value for you on upgrade.

Ready-for-merge Checklist

  • Expected files: all files in this pull request are related to one feature request or issue (no stragglers)?
  • Did you update or add any necessary documentation (README.md, WHY.md, etc.)?
  • Was this feature demo'd and the design review approved?
  • Did you update the CHANGELOG.md file with a summary of this update?

Be sure to share your updates with the patternfly-elements-contribute@redhat.com mailing list!

@castastrophe castastrophe changed the title [fix] Add a warning about updating the on attribute before upgrade fix: Add a warning about updating the on attribute before upgrade Mar 30, 2020
@starryeyez024
Copy link
Member

starryeyez024 commented Apr 1, 2020

👍 for testing and documentation, still needs JS review

much awesomeness:
image

@starryeyez024 starryeyez024 requested a review from kylebuch8 April 1, 2020 13:56
@castastrophe
Copy link
Contributor Author

@kylebuch8 Update applied! Working great!

@castastrophe castastrophe added ready: browser testing Test the component in the supported browser environments. bug labels Apr 6, 2020
@castastrophe
Copy link
Contributor Author

@kylebuch8 I updated the storybook instances to show theme support.

@castastrophe
Copy link
Contributor Author

@kylebuch8 Reverted the storybook updates and moved them into a separate branch. Prettier is still showing updates to the .storybook files but I can't undo that at this point.

Copy link
Contributor

@kylebuch8 kylebuch8 left a comment

Choose a reason for hiding this comment

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

Loans Get The Money

@kylebuch8 kylebuch8 merged commit f9c2813 into master Apr 15, 2020
@castastrophe castastrophe deleted the fix-add-warning-on-attribute branch April 15, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready: browser testing Test the component in the supported browser environments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants