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

2.x Prep and base #278

Open
wants to merge 41 commits into
base: develop
Choose a base branch
from
Open

2.x Prep and base #278

wants to merge 41 commits into from

Conversation

mikeethedude
Copy link
Contributor

@mikeethedude mikeethedude commented Jun 17, 2024

Summary

This is where we are starting for 2.x Here are some of the thigs we need to accomplish in this branch.

  • Remove eslint
  • Update or remove chalk
  • Migrate or rebuild as a module type package
  • Update testing requirements
  • Improve Help Text
  • Init command should be interactive instead of failing with missing parameters.
  • Fix github actions checks to conform with new stuff

Documentation update (required)

If this pull request requires a change to Emulsify documentation, those changes, updates, and/or new information must accompany this pull request.

How to review this pull request

  • Pull this code
  • Take note of node version
  • npm run build
  • npm run test - All tests should pass
  • npm link
  • Go to a drupal instance and make sure the node version is the same
  • emulsify init Should take you through a wizard to install
  • go to the theme directory and emulsify system install emulsify-ui-kit
  • Should have UI Kit installed
  • Try and install a component emulsify component install text --force
  • Should install without error!

Closing issues

Closes #

@mikeethedude
Copy link
Contributor Author

Holy cow this was hard to get committed. I had to ignore these tests for now. We must clean these up before this is ready:
init.test.ts log.test.ts jsonSchemaToTs.test.ts getInitSuccessMessageForPlatform.test.ts getPlatformInfo.test.ts getAvailableStarters.test.ts

@mikeethedude mikeethedude marked this pull request as ready for review September 25, 2024 18:17
@joetower joetower self-requested a review September 27, 2024 13:16
Copy link

@joetower joetower left a comment

Choose a reason for hiding this comment

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

@mikeethedude Should I be able to run:
emulsify component install blockquote --force

And have it, or any other component (https://github.com/emulsify-ds/emulsify-ui-kit/tree/main/src/components) I list, install successfully?

I tried installing the blockquote and I get no feedback

Screenshot from 2024-09-27 09-25-52

@mikeethedude
Copy link
Contributor Author

@mikeethedude Should I be able to run: emulsify component install blockquote --force

And have it, or any other component (https://github.com/emulsify-ds/emulsify-ui-kit/tree/main/src/components) I list, install successfully?

I tried installing the blockquote and I get no feedback

Screenshot from 2024-09-27 09-25-52

@joetower Only if that is defined here: https://github.com/emulsify-ds/emulsify-ui-kit/blob/main/system.emulsify.json
@robherba We should probably change the output to something helpful if we can't find the definition for that component.

@joetower
Copy link

@mikeethedude Okay, thank you!

The thing is, in testing this, those three already exist, which, I suppose is why the --force is needed. So, to properly test I should check the contents of text before running emulsify component install text --force and then after?

@mikeethedude
Copy link
Contributor Author

Yeah you can try and remove that component or otherwise mess it up to see if that puts the right one in.

@joetower
Copy link

@mikeethedude Thank you! I removed text and tried to re-install and this is what the cli returned

Screenshot from 2024-09-27 12-40-38

Note: it did add the text component, but also returned an unexpected links mention

@joetower
Copy link

@mikeethedude But, if I remove it again and run it with --force it does what you'd expect.

Screenshot from 2024-09-27 12-42-24

@mikeethedude
Copy link
Contributor Author

I believe that lines up with the expectations though I think it should be okay with a dependency existing. Maybe we should have it ask if you want to replace the dependency when it already exists? The red text is also probably a little strong there.

This is good feedback, thank you!

@joetower joetower self-requested a review September 27, 2024 17:48
Copy link

@joetower joetower left a comment

Choose a reason for hiding this comment

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

@mikeethedude This tests as you've outlined. Super excited about this release and what will come next. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

3 participants