-
Notifications
You must be signed in to change notification settings - Fork 0
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 plugin support to ngu-flow #8
Conversation
Can we add some description what we mean by plugin and which plugin we are going to support? |
@santoshyadavdev Yes, for now I'm just exploring how we can add plugin support so that i can be more versatile. Can you throw some idea about general plugin, how they designed? |
47d7863
to
8f45ba9
Compare
@santoshyadavdev can you check this proposal for plugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_functions generally are used for private functions, can you remove _ if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is a private functions only, only for testing purpose i just made in public. Do you have any alternative to access this functions in testcases
@@ -61,7 +68,7 @@ describe('Arrangements2', () => { | |||
'7': { x: 660, y: 600, id: '7', deps: ['5'] }, | |||
'8': { x: 660, y: 900, id: '8', deps: ['6', '7'] }, | |||
}; | |||
const actual = Object.fromEntries(arrangements.autoArrange()); | |||
const actual = Object.fromEntries(arrangements._autoArrange()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same her lets remove _ from functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for testing purpose made it public and added _
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it should be testes with public methods, call the public methods and check
Just added sonar cloud to this project as well |
@santoshyadavdev addressed most of the comments and also solved the issues reported by sonar |
Quality Gate passedIssues Measures |
@santoshyadavdev this PR is ready for review, can you do the final review once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should remove the private apis, but let me know if we want to merge this and want to take care of this later.
@santoshyadavdev for now we are good with the plugin implementation, later I'll change the testcases and avoid public methods. |
Cool let's merge it |
@santoshyadavdev Need your approval to merge |
Introducing Plugin support, main focus is to tree shake the features and easy to use
In this PR currently two plugins added.
Usage example
This is the basic example of the plugin usage.