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

prevent adding undefined to className when value do not provided #2551

Merged
merged 4 commits into from
Jan 11, 2021

Conversation

ubaidisaev
Copy link
Contributor

@ubaidisaev ubaidisaev commented Jan 6, 2021

Fix

Add default value to className param
image

Test

no test needed

set default value to className in order to prevent adding undefined when value don't provided.
set default value to className in order to prevent adding undefined when value don't provided
@dmsnell
Copy link
Member

dmsnell commented Jan 6, 2021

Hi @ubaidisaev - thanks for the commit!

I wonder if instead of passing an empty string we could avoid adding the class name altogether. If there's not a more elegant way this looks pretty good IMO.

How'd you find this?

@ubaidisaev
Copy link
Contributor Author

@dmsnell we can completely remove className property because it's not used right now, or we can use classnames package for conditionally joining classNames together.

@dmsnell
Copy link
Member

dmsnell commented Jan 6, 2021

@ubaidisaev we're already using classnames if you look deeper into the code.

still curious, and curiosity is all, how you stumbled upon this - it's a good catch but such a mundane detail 😄

@ubaidisaev
Copy link
Contributor Author

@dmsnell I know that classnames is used in the project and therefore suggested it.
I agree it's funny error but anyway )
I know there a lot more critical problems but i'm gonna go slow and familiarize myself with this project

@dmsnell
Copy link
Member

dmsnell commented Jan 6, 2021

@ubaidisaev I meant that in this specific case we're already using classnames where you suggested we add it

@ubaidisaev
Copy link
Contributor Author

ubaidisaev commented Jan 7, 2021

@dmsnell just check this demo
Maybe this way look more legit. It's all about avoiding adding undefined to markup.

@dmsnell
Copy link
Member

dmsnell commented Jan 7, 2021

@ubaidisaev I don't think I'm coming across clearly. if you look a couple lines under where you made the default value

https://github.com/Automattic/simplenote-electron/pull/2551/files#diff-23a991f90156f9d604d25223e49e6d1caa38297f75b7046fd226ad14fc131327R11

you will see the app is already using classnames as you are suggesting and probably the same change would occur without the default values to the arguments but by changing the call from className to [className]

@ubaidisaev
Copy link
Contributor Author

@dmsnell yeap you're right. I didn't notice it.

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

Thanks for updating with classNames usage!

@codebykat I'm turning this over to you to merge since I'm somewhat removed from the release process at the moment.

@codebykat
Copy link
Member

Should get into 2.5.0 just under the wire. Thanks @ubaidisaev!

@codebykat codebykat merged commit 4245952 into Automattic:develop Jan 11, 2021
@codebykat codebykat added this to the 2.5.0 milestone Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants