sibiraj-s / ngx-editor

🖋️ Rich Text Editor for angular using ProseMirror
https://sibiraj-s.github.io/ngx-editor/
MIT License
424 stars 183 forks source link

refactoring ngx-editor #11

Closed volodymyro-in6k closed 6 years ago

volodymyro-in6k commented 6 years ago

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).

sibiraj-s commented 6 years ago

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. :smile:

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

sibiraj-s commented 6 years ago

@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 commented 6 years ago

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

sibiraj-s commented 6 years ago

let me rearrange the directory structure first as we agreed :smile:

sibiraj-s commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

github-actions[bot] commented 2 years ago

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.