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

feat(screens): initialise package #4

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat(screens): initialise package #4

wants to merge 4 commits into from

Conversation

saulhardman
Copy link
Contributor

@saulhardman saulhardman commented Oct 12, 2021

Example usage: https://github.com/limbo-works/Empirical-Website-Shop-Umbraco-8/pull/367

1 place to configure both screens and breakpoints (screens are for TailwindCSS and breakpoints are for use throughout the JavaScript codebase).

Closes #1

 - @limbo-works/screens@0.0.1-alpha.0
Copy link
Member

@tfjordside tfjordside left a comment

Choose a reason for hiding this comment

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

I took it for a spin on empirical, changed a few breakpoints and it works as expected :)


const { breakpoints, screens } = configure([
{ value: 375, max: true },
{ value: 768, max: true },
Copy link
Member

Choose a reason for hiding this comment

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

Could we name this more explicitly? max could be confused(by me) to be something else.
"includeMaxWidth" or maybe "includeMaxScreen" since breakpoints always has both (minor)

breakpoints: {
...breakpoints,

[`${px}`]: { px, em, min, max },
Copy link
Member

Choose a reason for hiding this comment

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

breakpoints always include max. I think this is fine, as it'll be used with js and we might need this without a css breakpoint. (very minor)


const { default: configure } = require('@limbo-works/screens');

const { breakpoints, screens } = configure([
Copy link
Member

Choose a reason for hiding this comment

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

breakpoints and screens can be a bit confusing for people not familiar with tailwind screens. This can be alright as long as the docs show an example. (minor)

Copy link
Member

Choose a reason for hiding this comment

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

Wrote a comment on #1, but much to the same point - maybe naming of properties should be more explicit, like twScreens or something similar as suggested on the issue?

Don't know if breakpoints should be renamed to breakpointData or rawBreakpointData or something like that as well, or if breakpoints covers it well enough (very minor). But the tailwind specification would at the very least be nice.

But yeah, a lot can be explained through documentation as well. Though it will at to code readability, which is nice.


## Installation

Before installing the package, it's necessary to configure NPM with a GitHub
Copy link
Member

Choose a reason for hiding this comment

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

Question: Will our freelancers be able to do this?

Copy link
Member

@SEMilfred SEMilfred left a comment

Choose a reason for hiding this comment

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

I've added my notes additive to @tfjordside's - but I also approve as is, though it could be worth updating based on the feedback.

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.

Package(@limbo-works/screens): Standardise TailwindCSS (etc.) Breakpoint Configuration
3 participants