search

stevermeister / ngx-wig

Angular(also Angular 17) WYSIWYG HTML Rich Text Editor (from ngWig - https://github.com/stevermeister/ngWig)
Apache License 2.0
229 stars 51 forks source link

Prefixed css classes #180

Closed kalski closed 2 years ago

kalski commented 2 years ago

This is to allow using ngx-wig without poluting the CSS namespace.

We are using ngx-wig together with bulma dropdown and droprown and dropdown-content classes are available in both.

I am branching v13.1.6 because we use angular 13, so would appreciate if a 13.x version gets published in the end as well.

kalski commented 2 years ago

@bampakoa , I was concerned that using it would change the effectively applied styles to the elements with these prefixes. This definition is applied to all nw- prefixed className elements and its a new style for these elements.

However, I noticed something else - TButton.icon is a className extension point and if a new button icon is defined with its class name, say my-icon-class, now we will add the nwmy-icon-class class to the icon which is redundant. I'll put the icons prefixes in the library to avoid this.

kalski commented 2 years ago

@bampakoa , I resolved conflicts so you could merge this branch as you suggested here.

I would really need a 13.x version in the end.

bampakoa commented 2 years ago

Hey @kalski I still cannot understand clear why we can't use the nw- prefix instead. For example:

nwmenu-dropdown -> nw-menu-dropdown nwicon -> nw-icon

kalski commented 2 years ago

@bampakoa , here are the ordered list icon styles in several implementations:

I refer to the styles applied in the red rectangle, coming from here. They were not applied to the icons before but will be applied now if we choose nw-icon as a class name instead of nwicon or whatever name we choose without the nw- prefix

Visually I don't see any difference on the browser I test with but I am not sure if this will be the case for all the possible users of the library.

stevermeister commented 2 years ago

@kalski thank you! I'll try to publish new 13 version asap

stevermeister commented 2 years ago

@kalski could you please also update the tests? I see they failing

sonarcloud[bot] commented 2 years ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

stevermeister commented 2 years ago

@kalski I've created a separate branch - https://github.com/stevermeister/ngx-wig/tree/version13 could you make the same PR also for it? after we merge it and I'll publish it to npm also I'll keep this branch for a while if you need more updates - that we can manage it faster

bampakoa commented 2 years ago

@stevermeister can't we cherry-pick the commit from master into that branch directly?

stevermeister commented 2 years ago

@bampakoa you are welcome to assist, I have some doubts that I'll do it right

bampakoa commented 2 years ago

@stevermeister I just did it 🙂 Can you please confirm that the branch is ok now?

kalski commented 2 years ago

@bampakoa , I think new attributes in this change and this change also need to be excluded from the branch - they came when I merged master into the branch with this commit.

Here is a PR regarding this.

bampakoa commented 2 years ago

Good catch @kalski 👌 Thanks for pointing this out! That change was an improvement in the dropdown control with this PR. Do you think guys we should have it also in v13?

cc: @stevermeister

kalski commented 2 years ago

I'd rather not have it now, thanks.

stevermeister commented 2 years ago

@bampakoa @kalski indeed, we can keep it separate

stevermeister commented 2 years ago

@kalski Successfully published ngx-wig@13.2.6

one more time - thank you for your contribution!