Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

☂️ CSS Prefix: Missing Properties #1423

Closed
37 of 52 tasks
jer3m01 opened this issue Apr 8, 2021 · 76 comments
Closed
37 of 52 tasks

☂️ CSS Prefix: Missing Properties #1423

jer3m01 opened this issue Apr 8, 2021 · 76 comments
Labels
A-Compiler Area: compiler good first issue Good for newcomers L-CSS Language: CSS umbrella Issue to track a collection of other issues

Comments

@jer3m01
Copy link
Contributor

jer3m01 commented Apr 8, 2021

How To

Below is a list of properties missing from our current css prefix implementation.

Use ./script compiler-create-prefix [propertyName]. thanks @ematipico!
This will create:

  • internal/compiler/transforms/compile/css-handler/prefix/prefixes/[propertyName].ts
  • internal/compiler/transforms/compile/test-fixtures/css-handler/prefix/[propertyName]/input.css
  • website/src/docs/css-handler/prefix/[propertyName].md

They should all reference (using a link) at the top of their file (below imports) where the info came from.
Info like: property name, special values, ...

They should export an array of createPrefixVisitors and return a prefixCSSProperty or prefixCSSValue
(covers 99% use case, if not discuss prior).

export default [
  createPrefixVisitor({
    name: "<PROPERTY NAME>", // <PROPERTY NAME>/<VALUE NAME>
    enter(path) {
      return prefixCSSValue({
        path,
        propertyName: "<PROPERTY NAME>",
        value: "<VALUE NAME>", // Only for prefixCSSValue
        browserFeaturesKey: "<PROPERTY DATA NAME>", // From `caniuse-db`
      });
    },
  }),
  // ...
];

Prefixes will automatically be added to the property/value.

If some values/properties have special names (such as -moz-inline-flex being -moz-inline-box)
use the rename argument as show here and here.

Also be sure to test on https://autoprefixer.github.io/ for the same output. Don't forget to set target as > 0%.
And write tests for them in internal/compiler/transforms/compile/test-fixtures/css-handler/prefix/<PROPERTY NAME>/input.css

Examples

display, transform, transition.

Properties

Any -ms- (IE) only prefixes are not needed.

Reference: https://github.com/postcss/autoprefixer/blob/main/data/prefixes.js

Reserved means a rule isn't easy to implement and is reserved to be worked on by the team.
A ⚙️ indicates they are being worked on.
A checkmark indicates an open PR.

If any are missing, don't hesitate to comment!
& Thanks for all contributions! 💛

(note to myself) once complete, recheck the reference for any missed ones & reduce test sizes

@jer3m01 jer3m01 added good first issue Good for newcomers umbrella Issue to track a collection of other issues L-CSS Language: CSS A-Compiler Area: compiler labels Apr 8, 2021
@jer3m01 jer3m01 pinned this issue Apr 8, 2021
@ajkachnic
Copy link
Contributor

Currently working on supporting the box-shadow property

@ematipico
Copy link
Contributor

We should create a script file that creates and updates the prefix visitors. Like we do for create-lint-rule. I will take care of it.

@ajkachnic
Copy link
Contributor

ajkachnic commented Apr 8, 2021

I'm currently working box-shadow, and to support the browser prefixes, I'd need to do something like this, right?

export default ["box-shadow", "-webkit-box-shadow", "-moz-box-shadow"].map((
	value
) => {
	createPrefixVisitor({
		name: "box-shadow",
		enter(path) {
			return prefixCSSProperty({
				path,
				propertyName: value,
				browserFeaturesKey: "css-boxshadow",
			});
		}
	})
})

Or is there a better way to handle this using rename?

@jer3m01
Copy link
Contributor Author

jer3m01 commented Apr 8, 2021

["box-shadow", "-webkit-box-shadow", "-moz-box-shadow"]

is not required, prefixes are automatically added.
Just return a single createPrefixVisitor (same code as our discussion on discord).

I'll update the post to make it clearer.

Or is there a better way to handle this using rename?

rename is only used in the case that a specific browser uses a custom name instead of the generic one.

@ajkachnic
Copy link
Contributor

Ah alright, thanks for the help!

@ajkachnic
Copy link
Contributor

How should I handle running the tests while #1424 isn't merged? Because it just generates the exact same output at the moment because the new prefix hasn't been added to the main file.

Would it make sense to add it manually (and update the hash), or just wait until it's merged?

@jer3m01
Copy link
Contributor Author

jer3m01 commented Apr 8, 2021

It's just been merged 😄.

@ajkachnic
Copy link
Contributor

I've opened a pull request with box-shadow support!

@ajkachnic
Copy link
Contributor

I can work on filter. Since I know the process a lot more now, I should be able to add it fairly quickly

@magsout
Copy link
Contributor

magsout commented Apr 12, 2021

i'm working on the backface-visibility property. is that ok for you?

edit: hum, it's already part of https://github.com/rome/tools/blob/main/internal/compiler/transforms/compile/css-handler/prefix/prefixes/transform.ts#L27

so not need to do something more?

@magsout
Copy link
Contributor

magsout commented Apr 12, 2021

I opened a PR with position support, hope it's fine ;)

@jer3m01
Copy link
Contributor Author

jer3m01 commented Apr 12, 2021

edit: hum, it's already part of https://github.com/rome/tools/blob/main/internal/compiler/transforms/compile/css-handler/prefix/prefixes/transform.ts#L27

so not need to do something more?

Indeed thanks for the catch, I'll remove it from the list.

I opened a PR with position support, hope it's fine ;)

Great thanks!

@magsout
Copy link
Contributor

magsout commented Apr 13, 2021

I've opened a PR with box-sizing support

@magsout
Copy link
Contributor

magsout commented Apr 16, 2021

@jer3m01 @ematipico Do you mind if I make a pr with multiple properties?

@jer3m01
Copy link
Contributor Author

jer3m01 commented Apr 16, 2021

@jer3m01 @ematipico Do you mind if I make a pr with multiple properties?

No problem go ahead! As long as you include them clearly in the description of the PR.

@ktfth
Copy link
Contributor

ktfth commented May 5, 2021

I can work on user-select @jer3m01?

@jer3m01
Copy link
Contributor Author

jer3m01 commented May 15, 2021

These don't need prefixes:

* `touch-action`

* `overscroll-behavior`

* `unicode-bidi`

Thanks for the catch!

Although unicode-bidi does require some:

image

@sergeypriakhin
Copy link
Contributor

i will work on flow-into, flow-from, region-fragment

@imskr
Copy link
Contributor

imskr commented May 17, 2021

Working on unicode-bidi

@grimxreaper
Copy link

grimxreaper commented May 17, 2021

I can take a stab at text-decoration for my first contrib to this repo if that works? @jer3m01

Sure go ahead!

Hi there! I ran tests using ./rome test but I get this error ( InspectorClient has no active socket), any hint on what I need to do? I saw that everyone's test files (/input.test.md) was auto-generated so I'm guessing all I have to do to run tests is run the command since I have already created my input.css file and added all the styles. Am I missing something?

Screen Shot 2021-05-17 at 11 35 10 AM

@jer3m01 or anyone else who can help, I would really appreciate it as I am a beginner.

@ematipico
Copy link
Contributor

ematipico commented May 17, 2021

@grimxreaper The input.css is auto generate BUT you need to modify and add the correct test cases. The script can't guess the correct test cases.

Unfortunately the stack trace doesn't help. You could try to open a draft PR. From there we can help.

@grimxreaper
Copy link

grimxreaper commented May 17, 2021

@grimxreaper The input.css is auto generate BUT you need to modify and add the correct test cases. The script can't guess the correct test cases.

Unfortunately the stack trace doesn't help. You could try to open a draft PR. From there we can help.

@ematipico
Hm, how come I don't see those test cases in anyone's PR's in their input.css file?

@JustBeYou
Copy link
Contributor

@grimxreaper after writing your input.css you have to run rome test internal/compiler/transforms/compile/index.test.ts --update-snapshots and input.test.md will be auto-generated.

@ematipico I don't think it's stated anywhere in the docs how to update the snapshots. I was confused about this too a few months ago, maybe it would be useful to write it somewhere.

@grimxreaper
Copy link

Thank you @JustBeYou that was helpful, 2 problems were found when I ran the command you suggested. Could you show me an example of tests you or others have written on this thread for their CSS prefixes? I actually don't see examples of any tests or test files in the code base...any examples available?

@JustBeYou
Copy link
Contributor

JustBeYou commented May 17, 2021

@grimxreaper the idea here is not to write actual unit testing but to generate snapshots to automatically assure that the output does not change. Basically, the flow is as this:

  1. Write your code to add prefixes for some CSS props
  2. Write an input test case
  3. Generate the snapshots (that's basically running your code with your input and saving the output)
  4. Check the output by hand and/or using autoprefixer. If it is not ok, go back to 1, otherwise you're done

It would be useless to actually write the output for the test case by hand. Generating the output, checking it is correct, and storing it for future testing are enough for assuring that future code wouldn't break the current code. If you need more details you could check out the team's discord.

PS If I have a wrong understanding of the workflow, I'll be glad if someone from the core team could point it to me.

@sergeypriakhin
Copy link
Contributor

working on font

@lucasweng
Copy link
Contributor

lucasweng commented May 19, 2021

I will work on pseudo-classes

Edit:
Hi! After trying to implement pseudo-classes for some time, I found prefixing selector beyond me. I have found the following selectors in https://github.com/postcss/autoprefixer/blob/main/data/prefixes.js that need to be prefixed though.

Perhaps the selectors can be reserved to be worked on by the team as well? Or am I missing something? Could use some hint on what I need to do, thanks!

@JustBeYou
Copy link
Contributor

@lucasweng If it's alright, I could work on that as I have a rough idea about how it should be done. (I'm not from the core team btw)
I'll get back with a PR.

Working on pseudo-classes.

@lucasweng
Copy link
Contributor

@JustBeYou sure, and thanks for #1423 (comment)!

@reZach
Copy link

reZach commented May 21, 2021

Is there any more that need to be done here? I was going to take unicode-bidi but it appears @imskr has took it (the list hasn't been updated).

@dctalbot
Copy link
Contributor

@reZach can you help with background-origin and background-size? I mistakenly thought they weren't needed earlier

@lucasweng
Copy link
Contributor

Working on image-rendering.

@imskr
Copy link
Contributor

imskr commented May 23, 2021

@ematipico @jer3m01 Working on font and its derivatives. Also please update the unicode-bidi as I already created PR for it :)

@sergeypriakhin
Copy link
Contributor

@imskr Hi! The font is ready. I sent a pull request #1423 (comment)

@jamiebuilds jamiebuilds added this to Backlog in Compiling & Transforming via automation Jun 9, 2021
@jamiebuilds jamiebuilds moved this from Backlog to Planned in Compiling & Transforming Jun 9, 2021
@jamiebuilds
Copy link
Contributor

Hi! Thank you for opening this issue

We recently made the decision to migrate the Rome codebase to Rust and making some significant changes to its architecture. We've also decided not to release the previous version written in TypeScript.

We're going through and closing pull requests and issues that were specific to the TypeScript codebase, including this one. Thank you for taking the time to open it.

Thanks again!

Compiling & Transforming automation moved this from Planned to Done Sep 22, 2021
@ematipico ematipico unpinned this issue Sep 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Compiler Area: compiler good first issue Good for newcomers L-CSS Language: CSS umbrella Issue to track a collection of other issues
Projects
No open projects
Development

No branches or pull requests