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

class property was broken, like pugdiv( class="container" ) #111

Open
shirohana opened this issue Sep 6, 2019 · 14 comments
Open

class property was broken, like pugdiv( class="container" ) #111

shirohana opened this issue Sep 6, 2019 · 14 comments

Comments

@shirohana
Copy link
Contributor

shirohana commented Sep 6, 2019

As title, seems the class property alias was broken since ^7.0.0 but works in ^6.0.0.

It seems like the removal of alias class -> className in #62?

src/visitors/Tag.js
+// TODO: Need to drop all aliases for attributes
 switch (name) {
   case 'for':
     name = 'htmlFor';
     break;
   case 'maxlength':
     name = 'maxLength';
     break;
-  case 'class':
-    name = 'className';
-    break;
 }

Here's the sample:

It works when using className (REPL)

// import $ from './style.css'
const $ = { /* as CSSModule */ }

export function Link () {
  return pug`
    div( className=$.Container )
      a( className=[$.Link, $.LinkActive] )
  `
}

And it broken when class but it should be work (REPL)

// import $ from './style.css'
const $ = { /* as CSSModule */ }

export function Link () {
  return pug`
    div( class=$.Container )
      a( class=[$.Link, $.LinkActive] )
  `
}

Full log from my console

Module build failed (from ./node_modules/babel-loader/lib/index.js):
Error: /Users/shirohana/Repositories/react-spa-template/src/components/dev-tools/sitemap.js:41
    39|   return pug`
    40|     div( class=$.Container )
  > 41|       a( class=[$.Link, $.LinkActive] )
    42|   `
    43| }
    44|

We can't use expressions in shorthands, use "className" instead of "class"
    at makeError (/Users/shirohana/Repositories/react-spa-template/node_modules/pug-error/index.js:32:13)
    at Context.error (/Users/shirohana/Repositories/react-spa-template/node_modules/babel-plugin-transform-react-pug/dist/context.js:63:34)
    at node.attrs.map (/Users/shirohana/Repositories/react-spa-template/node_modules/babel-plugin-transform-react-pug/dist/visitors/Tag.js:81:23)
    at Array.map (<anonymous>)
    at getAttributes (/Users/shirohana/Repositories/react-spa-template/node_modules/babel-plugin-transform-react-pug/dist/visitors/Tag.js:45:28)
    at getAttributesAndChildren (/Users/shirohana/Repositories/react-spa-template/node_modules/babel-plugin-transform-react-pug/dist/visitors/Tag.js:125:17)
    at Object.jsx (/Users/shirohana/Repositories/react-spa-template/node_modules/babel-plugin-transform-react-pug/dist/visitors/Tag.js:166:35)
    at visitJsx (/Users/shirohana/Repositories/react-spa-template/node_modules/babel-plugin-transform-react-pug/dist/visitors.js:82:20)
    at nodes.forEach (/Users/shirohana/Repositories/react-spa-template/node_modules/babel-plugin-transform-react-pug/dist/visitors.js:58:19)
    at Array.forEach (<anonymous>)
@ezhlobo
Copy link
Member

ezhlobo commented Sep 6, 2019

@shirohana my intention was to avoid own aliases and just pass everything what we have to jsx. With class it's tricky since jsx didn't allow us to use it and now it throws warning, but it works well there. Maybe we should consider adding an alias for class attribute.

Do you prefer using class just because it's shorter or you have other reasons?

@shirohana
Copy link
Contributor Author

It it surprised me that the class has been removed without documented (´・ω・`)

The class should keep be available just because we love Pug more than JSX, and class is a Syntax of Pug, not an alias.

Just because we can use Pug as our preprocessor of JSX, we can avoid some bad design of JSX, like ugly optional rendering condition, ugly array iteration, meaningless closing-tag, and so many reasons that drive us to find a better workaround, that is why this plugin be existed.

@shirohana
Copy link
Contributor Author

We're looking for a solution, not another JSX-like alternatives. (´・ω・`ʃ♡ƪ)

@shirohana
Copy link
Contributor Author

Is it any plan for this (´・ω・`)?

@billypon
Copy link

@ezhlobo Could you give us an option that we can map class to className, default is not map? I prefer using class. Passthrough everything is right, because class and id are just html
attributes. But map class to className also is right, it can be a syntax of pug, and it's shorter. We are writing pug, not jsx, when we use this plugin.

@ezhlobo
Copy link
Member

ezhlobo commented Oct 7, 2019

@shirohana @billypon hey ✋, I feel sad that I made you waiting for too long.

I thought a bit about that and now I tend to agree that we need to have a map between class and className. I'll check it out tomorrow or after that.

Also, the message like We can't use expressions in shorthands, use "className" instead of "class" points to an issue in the logic, nothing about the avoidance of aliases.

@vanBrakel
Copy link

vanBrakel commented Oct 11, 2019

@ezhlobo In case you want to add the class -> className mapping back, then wouldn't it also make sense to add mapping for all other CSS attributes?

@shirohana
Copy link
Contributor Author

@JeanPaulVB These all other CSS attributes were created for JSX that makes almost DOM Element Attributes to be a valid JS Identifier (like variable names), but Pug is just a HTML-preprocessor, no need to be compatible with JavaScript, as I think (´・ω・`)

@vanBrakel
Copy link

vanBrakel commented Oct 12, 2019

@shirohana But the PUG attributes do need to be compatible with React right? Hence a pure Pug to JSX mapping (like class to className) could be very nice.

@shirohana
Copy link
Contributor Author

@JeanPaulVB Yeah that is what I expect this plugin does :D

✅I'd like that do:

.container
  label( for="email" )
  input#email( type="text" )

❌but not:

.container
  label( html-for="email" )
  input#email( type="text" )

❌and never:

.container
  label( htmlFor="email" )
  input#email( type="text" )

this plugin done that pretty well in this example, and I hope it can also applies on class -> className ❤️

@danielnarey
Copy link

Any plans to revert to allowing “class” (as in Pug language spec) in place of “className” (as in JS/JSX)?

@shirohana
Copy link
Contributor Author

shirohana commented Jun 18, 2020

Hi,
I'm back here for React and frontend ヾ(*´ω`*)ノ

Currently I have a plan to make a new option to allow using:

  • pug`div.box`,
  • pug`div( class="box" )`,
  • pug`div( className="box" )` and
  • pug`div( class=styles.box )` (for CSS Module)

in same time, just like alias that works well before.

with options like:

babel.config.js
{
  "plugins": [
    ["transform-react-pug", {
      "attributeAlias": {
        "KEY": "VALUE",
      }
    }]
  ]
}

Which:

  • KEY: Attribute name of HTML Element which is invalid in JSX and listed in React Doc.

    For example:

    • "class" replaced by className
    • "for" replaced by htmlFor
  • VALUE: The replacement string that should be valid in JSX.

This way provides the most flexibility of use cases, you can setup for JSX-like syntax, Pug-like syntax, and even more power that Pug doesn't have:

JSX-like

"attributeAlias": {
  /* No need to configure, unlisted attributes will be fallback as it is */
}

pug`div( className="box" )` // valid
pug`div( class="box" )` // invalid

pug`label( htmlFor="username" )` // valid
pug`label( for="username" )` // invalid

Pug-like

"attributeAlias": {
  "class": "className",
  "for": "htmlFor"
}

pug`div( className="box" )` // valid
pug`div( class="box" )` // valid, will be parsed to JSX `<div className="box"></div>`

pug`label( htmlFor="username" )` // valid
pug`label( for="username" )` // valid, will be parsed to JSX `<label htmlFor="username"></label>`

More power

"attributeAlias": {
  "@click": "onClick",
  "@change": "onChange",
  "@html": "dangerouslySetInnerHTML"
}

pug`button( @click=handleClick )` // valid, will be parsed to JSX `<button onClick=handleClick></button>`
pug`svg( @html=canvas )` // valid, will be parsed to JSX `<svg dangerouslySetInnerHTML=canvas></svg>`

Default alias will be like that for compatible with nature syntax of Pug:

"attributeAlias": {
  "class": "className",
  "for": "htmlFor"
}

I'll try to implement in few days, hope that can match everyone's preference easily.

Welcome for any advice ٩(。・ω・。)

@p-98
Copy link

p-98 commented Nov 15, 2021

@shirohana @ezhlobo Are there any updates or reasons why the PR is not merged?

@shirohana
Copy link
Contributor Author

@p-98 I have no idea, guessing this project was been deprecated with no announcements. and I'm gone for poor type checking from pug months ago 😉

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

No branches or pull requests

6 participants