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

style: created custom tagify scss file and updated gulp file #10063

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

IsaiahLevy
Copy link

@IsaiahLevy IsaiahLevy commented Mar 31, 2024

What

Implemented a custom SCSS file for Tagify inputs and modified the Gulpfile to include these styles in the build process, allowing for customizable appearance and integration with the project's frontend.

Related issue(s) and discussion

@IsaiahLevy IsaiahLevy requested a review from a team as a code owner March 31, 2024 20:45
@github-actions github-actions bot added CSS Template::Toolkit The templating toolkit used by product opener. The starting point for HTML/JS/CSS fixes. Site layout labels Mar 31, 2024
@IsaiahLevy IsaiahLevy changed the title created custom tagify scss file and updated gulp file Style: created custom tagify scss file and updated gulp file Mar 31, 2024
@IsaiahLevy IsaiahLevy changed the title Style: created custom tagify scss file and updated gulp file style: created custom tagify scss file and updated gulp file Mar 31, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.68%. Comparing base (dc04d18) to head (afbee76).
Report is 287 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10063      +/-   ##
==========================================
+ Coverage   49.54%   49.68%   +0.13%     
==========================================
  Files          67       71       +4     
  Lines       20650    20982     +332     
  Branches     4980     5026      +46     
==========================================
+ Hits        10231    10424     +193     
- Misses       9131     9266     +135     
- Partials     1288     1292       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

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

This is great, but I think we loose too much contrast on hover:

image

Also I would better use the brown which of the main theme instead of introducing a color which is not the right one.

@IsaiahLevy IsaiahLevy force-pushed the add-tagify-scss-customization branch from 7709e43 to 9a0467f Compare April 4, 2024 21:07
Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

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

@IsaiahLevy I still have a rule which does not work (on firefox)

But I found that there is a far better way to handle this:

You can just do:

@import "../node_modules/@yaireo/tagify/src/tagify.scss";

:root {
  $tagify-dd-color-primary: #341100 !important;
  --tagify-dd-color-primary: #341100 !important;
}

and it works (it does not work if I use $chocolate but I don't know why).
Do you want to try with that (there might be another variable to override)

}

// Hover state for dropdown items
.tagify__dropdown__item--active,
Copy link
Member

Choose a reason for hiding this comment

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

this rule does not seems to work, but I don't know why.

Copy link
Author

Choose a reason for hiding this comment

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

Hey thank you for the feedback. Just pushed changes to fix this.

@github-actions github-actions bot added the API Issues related to the Open Food Facts API. More specific labels exist & should be used (API WRITE…) label Apr 11, 2024
Copy link
Member

Choose a reason for hiding this comment

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

@IsaiahLevy your file has merging problems !

gulpfile.ts Outdated
@@ -133,16 +134,17 @@ function copyCss() {
"./node_modules/leaflet/dist/leaflet.css",
"./node_modules/leaflet.markercluster/dist/MarkerCluster.css",
"./node_modules/leaflet.markercluster/dist/MarkerCluster.Default.css",
"./node_modules/@yaireo/tagify/dist/tagify.css",
// "./node_modules/@yaireo/tagify/dist/tagify.css", // This line should be commented out or removed
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// "./node_modules/@yaireo/tagify/dist/tagify.css", // This line should be commented out or removed

Copy link
Member

Choose a reason for hiding this comment

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

This change looks out of place

@@ -1,4 +1,6 @@
@import "variables";
@import "custom-tagify";
@import "variables";
Copy link
Member

Choose a reason for hiding this comment

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

"variable" is already imported in line 1. Does it have to be included here, again?

@@ -19,6 +19,7 @@
<link rel="stylesheet" href="[% static_subdomain %]/css/dist/app-[% edq(lang('text_direction')) %].css?v=[% css_timestamp %]" data-base-layout="true">
<link rel="stylesheet" href="[% static_subdomain %]/css/dist/jqueryui/themes/base/jquery-ui.css" data-base-layout="true">
<link rel="stylesheet" href="[% static_subdomain %]/css/dist/select2.min.css">
<link rel="stylesheet" href="[% static_subdomain %]/css/dist/custom-tagify.css" data-base-layout="true">
Copy link
Member

Choose a reason for hiding this comment

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

We only really need Tagify in

<link rel="stylesheet" type="text/css" href="/css/dist/tagify.css" />
, so this probably needs to be removed, and the import in cgi/product_multilingual.pl should be updated.

@alexgarel
Copy link
Member

@IsaiahLevy, still you are overriding CSS rule, while I think we should only give values to variables as proposed in my comment

@IsaiahLevy IsaiahLevy requested a review from hangy May 1, 2024 13:25
Copy link

sonarqubecloud bot commented May 2, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

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

Great !

@alexgarel
Copy link
Member

@IsaiahLevy you have to fix linting, your changes to gulp file do not concord with lint policy.

make lint might fix it.

@github-actions github-actions bot added the 💥 Merge Conflicts 💥 Merge Conflicts label Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Issues related to the Open Food Facts API. More specific labels exist & should be used (API WRITE…) CSS 💥 Merge Conflicts 💥 Merge Conflicts multilingual products Site layout Template::Toolkit The templating toolkit used by product opener. The starting point for HTML/JS/CSS fixes.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Use tagify SCSS file for styling of tag inputs
6 participants