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

Adds CLI hooks (IDs) to certain elements #592

Merged
merged 3 commits into from
Nov 29, 2018
Merged

Conversation

mixn
Copy link
Contributor

@mixn mixn commented Nov 29, 2018

Hi guys. :) v3.6.0 introduced some breaking changes to the CLI I’m maintaining and trying to keep in sync (feature-wise).

I texted @mfix22 on Twitter about maybe adding some (non-changing) hooks for the CLI, so this doesn’t happen in the future any more. :)

I hereby added four ids for all the things the CLI accesses. Let me know if any changes are needed (maybe namespacing?)… :)

Thanks for being so responsive about this, hope to patch the CLI as soon as this is merged and deployed. :)

🙏

@vercel
Copy link

vercel bot commented Nov 29, 2018

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@mixn mixn changed the title Adds CLI hooks (ids) to certain elements Adds CLI hooks (IDs) to certain elements Nov 29, 2018
@mfix22
Copy link
Contributor

mfix22 commented Nov 29, 2018

@mixn thanks for this and sorry about breaking your tool 😬 I updated the id's to have a more consistent naming, and release. Hope that works!

@mfix22 mfix22 merged commit 09397ba into carbon-app:master Nov 29, 2018
@mixn
Copy link
Contributor Author

mixn commented Nov 30, 2018

@mfix22 No worries at all and thank you for the quick merge and deploy! 👏 Patching the CLI as we speak. Thanks again! 👍

@mixn mixn mentioned this pull request Nov 30, 2018
@mixn
Copy link
Contributor Author

mixn commented Jan 14, 2019

@mfix22 All the IDs except for export-container seem to be missing again. 😢

screenshot 2019-01-14 at 10 41 27

@mfix22
Copy link
Contributor

mfix22 commented Jan 14, 2019

@mixn apologies for this, I am pushing the updates now. Do you have time to add Cypress integration tests that test for the existence of these id's to our existing test suite?

@mfix22
Copy link
Contributor

mfix22 commented Jan 14, 2019

@mixn the release was just pushed — hopefully everything is fixed 👍

@mixn
Copy link
Contributor Author

mixn commented Jan 14, 2019

@mfix22 Thank you! ❤️ And I’ve seen #635 about the integration tests, will hopefully tackle it this weekend. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants