Navigation Menu

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

Feature - extend init command #135

Merged

Conversation

giothevanni
Copy link

@giothevanni giothevanni commented Aug 18, 2017

What does this PR cover?

This actually makes the tool more versatile, the way I see it there are two fundamentally different approaches for using astrum:

  1. Initializing the tool locally, commit all the generated files to be deployed in some way and served statically
  2. Initializing the tool locally, setting the files without project specific data (index.html, main(.min).js and style(.min).css) in the .gitignore and generate them via a CI Server and then deploy it.

By not being able to "re-init" astrum on an existing folder, the usage for group 2 is much more cumbersome. This pull request aims to solve that.

How can this be tested?

Initialize the library in any directory, and then do it again.

Screenshots / Screencast

astrum-extend-init


Reviewers

Review 1

  • 👍

Review 2 (optional)

  • 👍

By adding a +1 you are confirming you have...

  • Witnessed the work behaving as expected (this could be on the author's machine or screencast).
  • Checked for coding anti-patterns.

@giothevanni
Copy link
Author

I'll be looking forward to your feedback, when you get the chance, @RyanHavoc.

@giothevanni
Copy link
Author

Hey @RyanHavoc, haven't heard from you for a while, any chance you find some time to review this one?

@Andy-set-studio
Copy link
Contributor

Hey @giothevanni, great work!

I ran it on one of my instances and it worked as expected. One thing I would suggest though is that we use a more prominent colour for the message that shows when an existing instance exists. I'd suggest chalk.yellow. It'll be nice to keep such messaging nice and consistent throughout the manager.

What do you think @RyanHavoc?

Thanks again @giothevanni - we really appreciate your contribution.

@giothevanni
Copy link
Author

giothevanni commented Nov 24, 2017

Hey @hankchizljaw, thanks for the feedback!
I completely agree with you regarding color consistency and changed the color to yellow as you suggested.

Glad to help!

@RyanHavoc
Copy link
Member

Actually guys, in review, the message should be white. Yellow is used for feedback/informational messaging. If you look at the “Intuitive management from the command-line.” section on the homepage you can see the UI colours used. Anything interactive is white. Can you change back please?

@giothevanni
Copy link
Author

I'm torn on this one, on the one side you are totally correct that interactive prompts are white so far. On the other side this an interactive prompt with a warning aspect, for it would overwrite files (with a sensitive default to 'no') but that may still have undesired effects with some users, that's why I think that the color should not only be defined by interactive or non-interactive but by the nature of the message/prompt itself as well, but as you said, you already use yellow for feedback/informational, so it's already 'taken'

Maybe deliberate with @hankchizljaw for a couple of minutes this coming week and get back to me?

@RyanHavoc
Copy link
Member

We use red when deleting a component as it’s a destructive action. This would also work in this instance.

@Andy-set-studio
Copy link
Contributor

I think red would be a good shout. I certainly think there needs to be contrast to make sure that the user spots it and red means danger, which this situation certainly could be classed as.

Everyone happy with that then?

@giothevanni
Copy link
Author

giothevanni commented Nov 26, 2017

Oh man, what have I done... 😅
While my intention was to 'warn', I do want to point out again, that the files being overwritten are the ones generated by the library and the user shouldn't normally be tinkering around them, it's practically the same as doing an astrum update, that's why in my way of seeing things it would be in a scale of white/gray being trivial, yellow being a warning, and red being danger a yellow at most.

For those reasons I would plea to go back to white as @RyanHavoc suggested. I hope that works for you too, @hankchizljaw ?

@Andy-set-studio
Copy link
Contributor

To counter that though, I think customisations have been common because of some of the features not yet available with Astrum. A good example is when users want to implement SVG sprite sheets.

In that scenario, it would be dangerous to override.

I guess I’m accounting for accidents here.

@giothevanni
Copy link
Author

Fair enough, I'd be ok with red too, if it helps to prevent accidents. Will update this one shortly.

@Andy-set-studio
Copy link
Contributor

Safety first, @giothevanni 😉

Thank you!

@Andy-set-studio
Copy link
Contributor

Thanks @giothevanni, it looks great.

Because of the discussion, we'll do a double review on this one. When you're happy, can you merge it @RyanHavoc?

Thanks again @giothevanni, great work 🙂

@RyanHavoc RyanHavoc merged commit c5d784c into NoDivide:release/2.0.0 Nov 27, 2017
@RyanHavoc
Copy link
Member

Thanks @giothevanni!

@giothevanni
Copy link
Author

My pleasure!

@giothevanni giothevanni deleted the feature/extend-init-command branch November 27, 2017 11:31
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.

None yet

4 participants