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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug when passing multiple arguments to log #78

Closed
wants to merge 3 commits into from
Closed

Fix bug when passing multiple arguments to log #78

wants to merge 3 commits into from

Conversation

zant
Copy link

@zant zant commented Aug 28, 2019

hey!

really good playground, super nice, i already switch from jsbin to codepan 馃

i made this PR because i notice that when calling console.log with multiple arguments, the following error appears in the integrated console:

Screen Shot 2019-08-28 at 19 26 35

the error was due to the support of styling logs, i just check if any styling is intended, and if is not, it should return an unstyled text, i also added a blank when joining arguments to have them separated, but i'm not aware of the use case of the previous way

TODO:

  • Add support for arrays

please review @egoist @Prozi :)

@zant zant changed the title Fix bug when passing multiple arguments to log Fix bug when passing multiple arguments to log (WIP) Aug 29, 2019
@@ -200,6 +200,12 @@
}

const replacements = args[0].match(/(%[sc])([^%]*)/gm)

// If no replacement is found, we return a unstyled text
if (!replacements || replacements === null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (!replacements) {

is the same but shorter

Copy link
Contributor

@Prozi Prozi Aug 29, 2019

Choose a reason for hiding this comment

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

actually pleace change to

if (!replacements || !replacements.length) {

because we do replacements.shift() below that

anyway Thanks for contribution! 馃憤

Copy link
Author

Choose a reason for hiding this comment

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

just did the change!

Copy link
Contributor

@Prozi Prozi left a comment

Choose a reason for hiding this comment

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

1 minor change and good to go 馃檲

@@ -174,7 +174,7 @@
*/
const styleText = function (textArray, styles = []) {
return textArray.map((text, index) => {
return index ? `<span style="${styles.shift()}">${text}</span>` : text
return index ? `<span style="${styles.shift()}">${stringify(text)}</span>` : text
Copy link
Author

@zant zant Aug 30, 2019

Choose a reason for hiding this comment

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

@Prozi the item in WIP is referring to this:

Screen Shot 2019-08-30 at 10 15 25

because styleText is not expecting arrays at all, but if we stringify whatever arguments that are passed in, the function can handle and return those as strings properly

Screen Shot 2019-08-30 at 10 25 46

@zant zant changed the title Fix bug when passing multiple arguments to log (WIP) Fix bug when passing multiple arguments to log Sep 2, 2019
@zant
Copy link
Author

zant commented Oct 15, 2019

Can you do a follow up please? @Prozi

@zant zant closed this Jul 2, 2020
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.

None yet

2 participants