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(esl-media-query): ingore tuple values if query syntax passed #2452

Merged
merged 7 commits into from
Jul 18, 2024

Conversation

fshovchko
Copy link
Contributor

Closes: #2377

@fshovchko fshovchko added enhancement Improvements and additions to code needs review The PR is waiting for review labels Jun 7, 2024
@fshovchko fshovchko requested review from a team June 7, 2024 08:59
@fshovchko fshovchko self-assigned this Jun 7, 2024
@fshovchko fshovchko requested review from dshovchko, ala-n and NastaLeo and removed request for a team June 7, 2024 08:59
@fshovchko fshovchko requested a review from ala-n June 10, 2024 17:39
@fshovchko fshovchko changed the title feat(esl-media-query): ingore tuple values if query syntax passed (POC) feat(esl-media-query): ingore tuple values if query syntax passed Jun 11, 2024
@ala-n ala-n removed the needs review The PR is waiting for review label Jun 17, 2024
@fshovchko fshovchko added the needs review The PR is waiting for review label Jun 20, 2024
@ala-n ala-n requested review from a team and abarmina and removed request for a team July 4, 2024 10:08
@ala-n
Copy link
Collaborator

ala-n commented Jul 4, 2024

$${\color{red} NOT ACTUAL }$$

The only open questions (for voting I guess) is the new method name and signature.

For the context:
The new method is used to parse query according to its syntax and based on user input so both
parse ('1 | 2 | 3', TUPPLE_MEDIA) and parse('@XS => 1 | @SM => 2 | @MD => 3', TUPPLE_MEDIA) (TUPPLE_MEDIA provided but not used in that case) will be handled correctly.

The purpose of creating this method appeared

  • from the real project (where tupple with global media preset is used with potential usage of extended arrow syntax)
  • and in purpose to extend carousel API
      <esl-carousel media="@XS | @SM | @MD" count="1 | 2 | 3" ...>
      vs
      <esl-carousel count="1 | @SM => 2 | @MD => 3" ...>
    

Three is an existing method parse presented in ESLMediaRuleList but it can not be used to parse query in the mentioned way due to arguments order (array syntax and tupple media are the first arguments so it is mixed and depends on argument count)

  1. So first of all we need a clear name for new method:
    g. flexibleParse 0 1.5 1.5 1 0 1.5 5.5
    k. parseMixed 1.5 0 0 1.5 1.5 2 3 9.5
    b. addaptiveParse 1.5 1.5 1.5 1.5 1.5 0 1.5 9

    parseAny
    parseAuto

    a. parseAdaptive 0 0 0 1

    c. autoparse 0 0 0
    d. autoParse 0
    f. parseFlexible 0

    l. parseMixedValue 0

  2. The signature of the method is also a little bit questionable.
    I'd suggest to restrict it to

parseAdaptive(value: string, mask: string): ESLMediaRuleList;  
parseAdaptive<U>(value: string, mask: string, parser: RulePayloadParser<U>): ESLMediaRuleList<U>;  

We have a "low level" parse which accepts any argument count depending on what we are going to parse.
The new method's purpose is always to parse uncertain user input, so providing media tuple is mandatory in that case.

Please make me know if there is a concern with that point

FYI: @exadel-inc/esl-all-contributors

@fshovchko fshovchko added the postponed Postponed issue label Jul 8, 2024
@ala-n ala-n removed the needs review The PR is waiting for review label Jul 10, 2024
@fshovchko fshovchko removed the postponed Postponed issue label Jul 16, 2024
@fshovchko fshovchko changed the base branch from main to main-beta July 16, 2024 15:48
@fshovchko fshovchko changed the base branch from main-beta to main July 16, 2024 16:08
@fshovchko fshovchko force-pushed the feat/ingore-tuple-by-query branch 2 times, most recently from 7a6eef9 to 438c1a9 Compare July 16, 2024 19:41
@fshovchko fshovchko changed the base branch from main to main-beta July 16, 2024 19:41
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 13962 lines exceeds the maximum allowed for the inline comments feature.

Copy link

codeclimate bot commented Jul 16, 2024

Code Climate has analyzed commit 438c1a9 and detected 4 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 2
Duplication 2

The test coverage on the diff in this pull request is 61.1% (50% is the threshold).

This pull request will bring the total coverage in the repository to 64.7% (-0.5% change).

View more on Code Climate.

@fshovchko fshovchko requested a review from ala-n July 16, 2024 19:54
@fshovchko fshovchko added the needs review The PR is waiting for review label Jul 16, 2024
eslint/src/rules/4/deprecated.media-rule-list-parse.ts Outdated Show resolved Hide resolved
src/modules/esl-media-query/core/esl-media-rule-list.ts Outdated Show resolved Hide resolved
src/modules/esl-media-query/README.md Outdated Show resolved Hide resolved
@ala-n ala-n removed the needs review The PR is waiting for review label Jul 16, 2024
@fshovchko fshovchko requested a review from ala-n July 17, 2024 07:57
@fshovchko fshovchko added the needs review The PR is waiting for review label Jul 17, 2024
@ala-n ala-n added this to the ⚡ESL: 5.0.0 Major release milestone Jul 18, 2024
@ala-n ala-n force-pushed the feat/ingore-tuple-by-query branch from 93b4bed to 025166e Compare July 18, 2024 16:10
@ala-n ala-n merged commit 80f567a into main-beta Jul 18, 2024
7 checks passed
@ala-n ala-n deleted the feat/ingore-tuple-by-query branch July 18, 2024 20:57
@github-actions github-actions bot locked and limited conversation to collaborators Jul 18, 2024
@ala-n
Copy link
Collaborator

ala-n commented Jul 22, 2024

🎉 This issue has been resolved in version 5.0.0-beta.25 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Improvements and additions to code needs review The PR is waiting for review released on @beta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🚀esl-media-query]: (POC) consider out of the box support for esl-media-query-list syntax recognition
5 participants