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

Try template-css in DragDrop plugin #239

Closed
wants to merge 1 commit into from
Closed

Conversation

arturi
Copy link
Contributor

@arturi arturi commented Jun 29, 2017

This PR tries to silently utilize template-css in DragDrop
plugin only. DragDrop styles are removed from uppy.css bundle and added through JS instead.

This also opens a door to themeable UI and shared (style) variables in JS, so we can use them for inline CSS or regular class-css or wherever:

style () {
  const settings = {
    fontSizes: {
      normal: '1.2em'
    },
    colors: {
      gray: '#ccc'
    }
  }

  css`
    .UppyDragDrop-container.drag {
      border-color: ${settings.colors.gray};
    }

    .UppyDragDrop-label {
      display: block;
      cursor: pointer;
      font-size: ${settings.fontSizes.normal};
    }
  `
}

Preprocessors via babel plugin and postcss are not utilized yet.

This PR silently tries to utilize
[template-css](https://github.com/arturi/template-css) in DragDrop
plugin only. DragDrop styles are removed from uppy.css bundle and added
through JS instead.

This also opens a door to themable UI and shared (style) variables in
JS, so we can use them for inline CSS or regular class-css or wherever:

```js
style () {
  const settings = {
      fontSizes: {
        normal: '1.2em'
      },
      colors: {
        gray: '#ccc'
      }
    }

   css`
     .UppyDragDrop-container.drag {
          border-color: ${settings.colors.gray};
        }

     .UppyDragDrop-label {
        display: block;
        cursor: pointer;
        font-size: ${settings.fontSizes.normal};
      }
  `
}
```

Preprocessors via babel plugin and postcss are not utilized yet.
@arturi arturi requested a review from goto-bus-stop June 29, 2017 01:04
}
}

css`
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder why Github isn't highlighting this, I thought it also did that for css tags 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I remember that too.

@goto-bus-stop
Copy link
Contributor

Some potential issues with this approach:

  • Themes in JS, but with predictable classnames, means there can only be one theme active on the page at any time. So if you have multiple Uppy instances (say two instances with DragDrop and different endpoints) with different theme colours one would override the other…
  • Having style() called multiple times on the same page would add the CSS twice. We could move the css`` tag outside the class so it only runs once on load but then we also don't have JS theming.

Maybe we could do something like this, splitting up the base styles and the themed styles:

// Add base CSS once on load
css`
  .UppyDragDrop {
    // default styles
  }
`

class DragDrop extends Plugin {
  constructor () {
    // …
    this.themeId = somethingUnpredictable()
  }
  style () {
    // add themed stuff here
    this.css = css`
      .UppyDragDrop-${this.themeId} {
        border-color: ${this.opts.theme.borderColor};
      }
    `
  }

  render () {
    return `<div className="UppyDragDrop UppyDragDrop-${this.themeId}"> ... </div>`
  }

  install () {
    this.style()
    // ...
  }
  uninstall () {
    this.css.removeTheThemeCssSomehow()
  }
}

Or we could keep it simpler without the JS-based theming for a start … or maybe there's a way to do it that I'm not seeing 🤷‍♀️

@arturi
Copy link
Contributor Author

arturi commented Jun 30, 2017

Yes, valid points. Thanks! I’ll get back to this after the release then, it seems we better hold this off for a little longer.

@goto-bus-stop
Copy link
Contributor

Dropping this here so we remember: https://www.npmjs.com/package/postcss-prefix-selector

@arturi
Copy link
Contributor Author

arturi commented Feb 5, 2018

Closing for now, it’s been open for two long with no activity.

@arturi arturi closed this Feb 5, 2018
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.

2 participants