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.

Sign up for GitHub

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

Jump to bottom

Features: #123 Array like options #124

Merged
merged 4 commits into from
Dec 6, 2023
Merged

Features: #123 Array like options #124

merged 4 commits into from
Dec 6, 2023

Conversation

stephane-segning
Copy link
Contributor

No description provided.

@vercel Vercel
Copy link

vercel bot commented Dec 1, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
medusa-plugins ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 4, 2023 10:49am

@stephane-segning stephane-segning force-pushed the features/123-array-like-options branch from 6f5c7f8 to 27c6fc1 Compare December 1, 2023 16:07
@stephane-segning
Copy link
Contributor Author

@adrien2p please test this and tell me :)

@adrien2p
Copy link
Owner

adrien2p commented Dec 1, 2023

I ll do it as soon as I can 😊 thanks a lot. We will also have to update the documentation to mention that you can now pass an array of object/function. I think ill also do some helpers for the test for example the container mock and things like that

@stephane-segning
Copy link
Contributor Author

Good ideas!

  • Implementing new examples would be much more helpful for newbies.
  • Having better doc is a foundation for us but for other developers too for having good code. Tell me when you're doing, I'll like to help :)

@adrien2p
Copy link
Owner

adrien2p commented Dec 4, 2023

If you have any idea to make the doc better i would be glad to look at your suggestions 🎉very happy to have you help me on the plugin. As you know i am pretty busy and i am trying to find time to look at your current prs 🤣

adrien2p
adrien2p reviewed Dec 4, 2023
View reviewed changes
Copy link
Owner

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

Could you run the lint script please to remove all lint issues and simplify the review 💪

packages/medusa-plugin-auth/src/api/index.ts Show resolved Hide resolved
packages/medusa-plugin-auth/src/loaders/index.ts Outdated Show resolved Hide resolved
@stephane-segning @adrien2p
bump: simplify the array extraction
fabab9a 
Co-authored-by: Adrien de Peretti <adrien.deperetti@gmail.com>
@stephane-segning
Copy link
Contributor Author

@adrien2p please review it again

@adrien2p
Copy link
Owner

adrien2p commented Dec 5, 2023 •
edited
Loading

Yes i will as soon as i can, i do it in my free time and i am a dad as well while i work at medusa 😉

Ill review it by the end of the day

adrien2p
adrien2p approved these changes Dec 5, 2023
View reviewed changes
Copy link
Owner

@adrien2p adrien2p left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, have you been able to test with some strategies? for me it looks good, once I merge it we can focus on the doc part of it wdyt?

@stephane-segning
Copy link
Contributor Author

Ive not tested it yet through and through. Still doing it though, but I'm not having errors till now. So I believe it's ok.

Also working on a doc directly after is a good idea. Don't just deploy it for now 😊

@adrien2p adrien2p merged commit 65e7439 into adrien2p:main Dec 6, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrien2p adrien2p adrien2p approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stephane-segning @adrien2p