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

Allow the user to use custom shapes and enable/disable the border #42

Merged
merged 15 commits into from
Aug 12, 2021

Conversation

DouglasdeMoura
Copy link
Contributor

@DouglasdeMoura DouglasdeMoura commented Jun 10, 2021

Changes:

  • Add support to CSS filters;
  • Add support to custom shapes with CSS clip-path1. When a clip-path is defined, the rounded setting is ignored;
  • Add option to enable/disable border;
  • Reorganizes the CSS for better readability;
  • Refactor render method to allow future extensions.

@DouglasdeMoura DouglasdeMoura changed the title Add option to enable/disable border Allow the user to use custom shapes and enable/disable the border Jun 10, 2021
@diego3g diego3g added the enhancement New feature or request label Jun 10, 2021
@diego3g
Copy link
Collaborator

diego3g commented Jun 11, 2021

Will take a deeper look as soon as possible! Thanks for the contribution <3

Copy link
Collaborator

@diego3g diego3g left a comment

Choose a reason for hiding this comment

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

Hey @DouglasdeMoura, first of all, thanks for the contribution!

For the next contributions, please try to send one PR per new feature.

Besides that, it would be really helpful if you could provide more information on the PR like what happens when clipPath is not set and does the shortcut to switch from rounded and square window still works with this new setting? If it works, how it works?

I think we should not rename borderColorCss to borderColor for now as it'll cause a breaking change so people that have already set that setting will be affected, maybe we can change this one in a future release?

I think that we shouldn't add the filter setting for now, it's a feature that I want to give more attention and not provide it just as a single setting for the final user, I want to provide already predefined filters and custom controls like brightness, saturation, contrast, etc, so user does not have to search through CSS filter docs.

Finally, taking a look at your src/cam.ts file, why did you change the way the file works by adding or removing classes during the render method execution? I think I prefer the old style where we add the classes and transforms just a single time after calculating its values based on user settings, like this:

this.videoElement.style.transform = transform.join(' ')
this.videoElement.className = classList.join(' ')

TLDR:

  • Let's keep this PR just for the new clipPath and disable border setting;
  • Let's keep borderColorCss as the name of borderColor setting for now;
  • Let's remove the filter setting as a I want to give it more attention;

Please, any doubts touch me so we can improve this even more! 😃

@DouglasdeMoura
Copy link
Contributor Author

Hello @diego3g,

When clipPath is set, the rounded setting is ignored, but, as you pointed it, maybe it would be better to toggle square/rounded/clipPath, what you think?

About the variable renaming, should not be a great deal of change, given the current limited user base, but yes, should be better managed in a major release.

As clipPath is taken directly from the CSS specification, would not be better to keep the filter setting working on the same way? Maybe, just provide a table of good filters for the users to copy and paste. Also, for future releases, it would be better to have an interface to preview such custom filters, and not giving them different names on the settings file.

I think it's better to use the methods we already have than to reimplement them, as it was being made before.

@diego3g
Copy link
Collaborator

diego3g commented Jun 14, 2021

Maybe the clipPath could be rounded by default so user can change the window format by changing this property and then, the shortcut will toggle between clipPath and square by default, what do you think?

Let's not rename the borderColor for now, will certainly include this in a next major release.

My worry about the filter setting is that once we include it we gonna have to support it on the next releases and I don't feel that comfortable adding this setting as it's not yet documented or close to a desired approach where the user is guided on how to use it instead of searching the CSS filter docs.

What do you think about removing the filter for now and then we take some time discuss a little bit how would be the best approach? I have some ideas and then you can implement it and send a new PR 🥰. I really think that this setting will be a game changing for Mini Video Me app and we should give all the attention it needs.

@maykbrito
Copy link
Owner

Hi @DouglasdeMoura !! Thanks for your contribution. 🥰
Awesome work here! ⚡︎

Let me add some points to this.

When clipPath is set, the rounded setting is ignored, but, as you pointed it, maybe it would be better to toggle square/rounded/clipPath, what you think?

I like it. At least, we do not lose the toggle option. 💜

About this PR. I prefer that it can only add clipPath and nothing more. Then, about other incredible features that you add here, you can open another PRs later.

Oh.. And I'm missing some documentation for using clipPath. It will be great if you could provide some instructions or examples in README.md. Could you?

Let me know if it's comfortable for you!
Thanks again @DouglasdeMoura

@diego3g
Copy link
Collaborator

diego3g commented Jun 18, 2021

Hey @DouglasdeMoura, will you be able to send the adjustments from our reviews? If not, no problem, i think i can get some of the code from this PR and add the adjustments to have the clip path setting.

@DouglasdeMoura
Copy link
Contributor Author

Yes, I'll be doing it today. I've been busy this week. Tonight I'll be working on it :)

@DouglasdeMoura
Copy link
Contributor Author

People, sorry for the delay, I've pushed the requested changes (borderColor was rolled back to borderColorCss and dropped the filter support). I decided to keep the border toggle because, for some shapes, borders do not work well. Please, give me your feedback as soon you guys see fit :)

@diego3g diego3g self-requested a review August 11, 2021 21:00
src/cam.ts Show resolved Hide resolved
@diego3g diego3g requested a review from maykbrito August 12, 2021 12:53
@maykbrito
Copy link
Owner

@DouglasdeMoura thanks for your effort man! 😍✌🏽

@maykbrito maykbrito merged commit 4851c42 into maykbrito:master Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants