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

Add modifiers to context.getClassList result #10251

Merged
merged 5 commits into from
Jan 26, 2023
Merged

Conversation

bradlc
Copy link
Contributor

@bradlc bradlc commented Jan 5, 2023

This PR adds modifier information to the context.getClassList result. This will allow IntelliSense to provide completions for modifiers such as those for line-height.

Example result:

[
  // ...
  'uppercase',
  'lowercase',
  // ...
  [
    'text-2xl',
    {
      modifiers: ['3', '4', '5', '6', '7', '8', '9', '10', 'none', 'tight', 'snug', 'normal', 'relaxed', 'loose'],
    },
  ],
  // ...
]

@bradlc bradlc requested a review from RobinMalfait January 5, 2023 15:43
@bradlc bradlc changed the title Add modifiers to conext.getClassList result Add modifiers to context.getClassList result Jan 5, 2023
Copy link
Member

@RobinMalfait RobinMalfait left a comment

Choose a reason for hiding this comment

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

Looking good!

@RobinMalfait
Copy link
Member

One thing I wonder though: will this be a breaking change for people using an older version of the intellisense plugin but using a newer version of Tailwind? If so, should we add a flag to the function instead? 🤔

@bradlc
Copy link
Contributor Author

bradlc commented Jan 6, 2023

One thing I wonder though: will this be a breaking change for people using an older version of the intellisense plugin but using a newer version of Tailwind? If so, should we add a flag to the function instead? 🤔

It would break in that case, yeah. Do you mean something like context.getClassList({ includeMetadata: true })?

@bradlc
Copy link
Contributor Author

bradlc commented Jan 10, 2023

What do you think of this now @RobinMalfait?

@RobinMalfait
Copy link
Member

Yep I think that would work!

But I'm still trying to figure out how it would work from the plugins perspective and if we won anything with this includeMetadata change or not haha 😅

The scenario's are:

  • Up to date plugin & tailwind version
    The plugin expects the metadata, tailwind provides the metadata
  • Up to date plugin & old tailwind version
    The plugin would provide the includeMetadata, but tailwind won't return it. So I guess you have to check if it returned the meta data or not. Or you could also check the version number up front? We could return an object like { includesMetadata: true, data: [] } but you would still need to check it and if we change the layout of the data returned it would break for older versions.
  • Old plugin & up to date tailwind version
    This is where the includeMetadata will be useful I think. The plugin won't provide the flag (because older version) so tailwind won't return it.
  • Old plugin & old tailwind version
    Current scenario, so all good!

Is that correct?

@bradlc
Copy link
Contributor Author

bradlc commented Jan 17, 2023

Yeah that sounds right. For the second scenario what the extension currently does is roughly result.some(item => Array.isArray(item)) to see if tailwindcss returned metadata. If there's an array value in there then we have metadata. I think this is sufficient, but I'm wondering if we should always return arrays if includeMetadata is true? Something like:

[
  ['uppercase'],
  ['text-2xl', { modifiers: [/* ... */] }],
  // ...
]

Then for the "check" you could do something like Array.isArray(result[0]). What do you think?

@RobinMalfait
Copy link
Member

@bradlc Going to respond here so that it is a bit more public.

Always using an array is an option, but you still have to check whether or not the options and modifiers exists.

I personally don't mind the mix between strings and arrays, because I see it like strings and tuples where if you want to configure options then you can switch to a tuple.

I think the important part here is that we choose something that is the easiest for you to use without running into (too many) breaking changes.

@bradlc bradlc merged commit ef6f9ee into master Jan 26, 2023
@bradlc bradlc deleted the getclasslist-modifiers branch January 26, 2023 10:58
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.

4 participants