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

Feat/add cli #22

Merged
merged 8 commits into from
Aug 16, 2023
Merged

Feat/add cli #22

merged 8 commits into from
Aug 16, 2023

Conversation

0x1337ak
Copy link
Contributor

@0x1337ak 0x1337ak commented Jul 13, 2023

hi @SihamBen @yjose I need your feedback, is this the right path to walk in?
obytes-cli
and also for managing the packages i think about using https://www.npmjs.com/package/lerna and keep the cli separate from the app-icon-badge

what does this do

  • add cli feature

@0x1337ak 0x1337ak marked this pull request as draft July 13, 2023 10:16
@0x1337ak 0x1337ak requested review from yjose and SihamBen July 13, 2023 10:26
@yjose
Copy link
Member

yjose commented Jul 13, 2023

@0x10Ayoub here is my thought on this:

  • I don't think we need to create a separate module for this. Using a separate module means we'd have to use a monorepo tool to manage it, which I don't think is necessary in our case. I would recommend just creating a separate folder cli and putting the CLI logic there.
  • Regarding the file configuration, I'm not sure if requiring a configuration file is the best option to be honest. I would prefer a simple command even if it seems a little bit complex in our case, for that reason let's start small regarding this. The CLI should only allow adding one badge to the icon and should be similar to the following:
npx app-icon-badge  icon.png \
  --type=ribbon\
  --text=text\
  --position=top \
 --background-color=#fff \
 --color=#000 

let me know what you think ?

@0x1337ak
Copy link
Contributor Author

0x1337ak commented Jul 13, 2023

@yjose regarding the config file it is optional instead of passing parameters as you mention we let user write them in json file
example

[
  {
    "icon":"../example/icon.png",
    "badges": [
      {
        "text": "qa",
        "type": "banner",
        "color": "black",
        "position":"top",
        "background": "#3300ff"
      },
      {
        "text": "V1.0.1",
        "type": "ribbon",
        "position":"left"
      }
    ]
  },
  {
    "icon":"../example/adaptive-icon.png",
    "isAdaptiveIcon": true,
    "badges": [
      {
        "text": "prod",
        "type": "banner",
        "color": "black",
        "position":"top",
        "background": "#ff0099"
      },
      {
        "text": "V1.0.1",
        "type": "ribbon",
        "position":"left"
      }
    ]
  }
]

for passing parameters like i think it will be hard to add multiple icons with different parameters that's why i suggested 3 different why to get user input

  • json file
  • non-interactive passing paramaters
  • interactive (less favorite)
npx app-icon-badge  icon.png \
  --type=ribbon\
  --text=text\
  --position=top \
 --background-color=#fff \
 --color=#000 

@yjose
Copy link
Member

yjose commented Jul 13, 2023

ok great make sense. to make sure we are in the same path i would recommend making non-interactive passing paramaters work as expected then we can make other option too.

@0x1337ak
Copy link
Contributor Author

0x1337ak commented Jul 13, 2023

just as side note i did start with json file running validation with zod

@0x1337ak 0x1337ak marked this pull request as ready for review August 8, 2023 11:40
@0x1337ak
Copy link
Contributor Author

0x1337ak commented Aug 8, 2023

What does this do?

  • add cli feature for the default command
npx app-icon-badge  icon.png \
  --type=ribbon\
  --text=text\
  --position=top \
 --background-color=#fff \
 --color=#000 

How did you test this?

Locally manually with build and npm link

Copy link
Collaborator

@SihamBen SihamBen left a comment

Choose a reason for hiding this comment

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

👍

@SihamBen SihamBen merged commit 416d2e2 into master Aug 16, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants