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

refactoring ngx-editor #11

Merged
merged 18 commits into from
Nov 22, 2017
Merged

refactoring ngx-editor #11

merged 18 commits into from
Nov 22, 2017

Conversation

volodymyro-in6k
Copy link
Contributor

@volodymyro-in6k volodymyro-in6k commented Nov 20, 2017

Hello, @sibiraj-s.
I'd like your library and I'd like to help u.

This pull request, it's just refactoring and there are not new features. It can be a start point :)

I've temporarily removed package.json precommit step, as it cannot build it properly. Any way we can fix it and revert (package.json precommit step).

volodymyro-in6k and others added 11 commits November 18, 2017 18:05
Refactoring:
 - Generate ngx-toolbar-component
 - Move toolbar code into the ngx-toolbar-component
 - Generate ngx-editor-message
 - Move message code into the ngx-editor-message
Provide messages exceptions handling
Encapsulate message auto clean on 5 seconds
angular/cli v1.5.3 fixes a security vulnerability that was reported for Handlebars
@sibiraj-s sibiraj-s changed the title Patch 2 refactoring ngx-editor Nov 21, 2017
@sibiraj-s
Copy link
Owner

sibiraj-s commented Nov 21, 2017

I'd like your library and I'd like to help u.

Thanks for the PR @volodymyro-in6k.

Wooo, That's a lot of changes. 😄

When I need this component I was in a hurry so I created that just focussing on functionality. Since issue #9. I started refactoring ngx-editor. Since a lot of work has been done in the PR. we can continue from here.

Also, following angular commit style guide for the commit message would be better, In future, it may help in generating changelogs

@sibiraj-s sibiraj-s added Type: Enhancement Improves existing functionality Type: Refactor Change in code without any functional changes labels Nov 21, 2017
@sibiraj-s
Copy link
Owner

Can we refactor the directory structures as

.
└── ngx-editor
    ├── components
    │   ├── ngx-editor-message
    │   ├── ngx-editor-toolbar
    │   └── ngx-grippie
    ├── others
    │   └── ngx-editor.utils.ts
    │   └── ngx-editor.defaults.ts
    └── services
        └── command-executor.ts

or any other suggestion?

@volodymyro-in6k
Copy link
Contributor Author

Thanks for the guide :)
@sibiraj-s the structure can be refactored, the way you have described above.
Also, as an option, it can have the following structure

ngx-editor/
├── common
│   ├── models
│   ├── services
│   │   ├── command-executor.ts
│   │   └── message.service.ts
│   └── utils
├── ngx-editor.component.html
├── ngx-editor.component.scss
├── ngx-editor.component.ts
├── ngx-editor.defaults.ts
├── ngx-editor-message
│   ├── ngx-editor-message.component.html
│   ├── ngx-editor-message.component.scss
│   └── ngx-editor-message.component.ts
├── ngx-editor.module.ts
├── ngx-grippie
│   ├── ngx-grippie.component.html
│   ├── ngx-grippie.component.scss
│   └── ngx-grippie.component.ts
└── ngx-toolbar
    ├── ngx-editor.utils.ts
    ├── ngx-toolbar.component.html
    ├── ngx-toolbar.component.scss
    └── ngx-toolbar.component.ts

The idea to don't divide it into the components, services, etc. Let's keep it as the components structure for the better navigation. E.g. if ngx-toolbar will have an inner component, then it's going to be located under the ngx-toolbar dir.

As for the utils, models, and services. We can use the following strategy: keep files together (near the component) when it uses only in the one directory (component) level. Or extract it to the (shared, common or smth else) if it uses in the several places e.g. messages.service.

What do you think?

@sibiraj-s
Copy link
Owner

ngx-toolbar will have an inner component, then it's going to be located under the ngx-toolbar dir.

That makes sense. Then we can have the same as you proposed.

@volodymyro-in6k
Copy link
Contributor Author

@sibiraj-s, could you provide me with an additional information about the contribution flow?
1 feature per 1 branch?

@sibiraj-s
Copy link
Owner

sibiraj-s commented Nov 21, 2017

@volodymyro-in6k . 1 feature/patch per branch will be great.

I haven't thought of contribution flow yet. But I soon will come up with one.

Since this PR includes commits messages that don't fall under angular commit guidelines. I was thinking of squash merging it. Instead of making a merge commit.

You added the form control support. that was great. Let us keep that for another PR. We will continue to refactor this one and then can add the features on that.

Is it Okay?

@sibiraj-s
Copy link
Owner

Can you revert that FEATURE? We can add it in another PR

@sibiraj-s
Copy link
Owner

sibiraj-s commented Nov 21, 2017

let me rearrange the directory structure first as we agreed 😄

@sibiraj-s
Copy link
Owner

@volodymyro-in6k . Isn't this almost over. One last thing that has to be reviewed is the namings. Then this PR can be merged.

@sibiraj-s
Copy link
Owner

@volodymyro-in6k . I have made significant changes. If you are fine with those changes. These can be merged and further can be continued from that.

@volodymyro-in6k
Copy link
Contributor Author

volodymyro-in6k commented Nov 21, 2017

@sibiraj-s great work :)
I'm to able to create PR on the cross PR.
So, I've created separated branch PR for formControl support.
Here is the fix of #9 PR
I'm sure that it's not the better way to do it, but this way it works for both formControl and @input changes from outside. (I'd like to refresh content through the ngOnChanges in both cases formControl and Input changes)

@sibiraj-s sibiraj-s merged commit b707409 into sibiraj-s:master Nov 22, 2017
@sibiraj-s
Copy link
Owner

@volodymyro-in6k. That PR is not here. it is in your repo.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in the thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Enhancement Improves existing functionality Type: Refactor Change in code without any functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants