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

Update to use CSS vars #2561

Merged
merged 7 commits into from
Feb 24, 2022
Merged

Conversation

Rahmon
Copy link
Contributor

@Rahmon Rahmon commented Jan 19, 2022

Description of the Change

#2489

Alternate Designs

Possible Drawbacks

Verification Process

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

Credits

Props @

@Rahmon Rahmon added this to the 4.0.0 (beta 2) milestone Jan 19, 2022
@JakePT JakePT self-requested a review January 24, 2022 13:09
@JakePT
Copy link
Contributor

JakePT commented Jan 25, 2022

@felipeelia Even though the variables are scoped to #ep-sync-page, they should probably still be prefixed. So --color-blue-01 would be --ep-admin-color-blue. At that point I would recommend bringing them up to :root and using them throughout the ElasticPress admin, rather than just the sync page.

@mehidi258 mehidi258 self-assigned this Jan 25, 2022
@mehidi258
Copy link
Contributor

@felipeelia I have added prefix for all variables

@felipeelia
Copy link
Member

Great @mehidi258 , thanks! But I think that as we are moving them to :root, we probably should make it available for all the other files, i.e., perhaps we should have a vars.css imported before all the others CSS in the plugin. Thoughts, @JakePT?

@JakePT
Copy link
Contributor

JakePT commented Jan 27, 2022

@mehidi258 Yeah I agree with @felipeelia, I think it makes sense to put this into a shared config file that's imported into the other admin files too. With that usage in mind I'm thinking it makes sense to adjust some of the variable names. For example, I don't think we need the -01 on the color names, and some of the variables that are specific to the sync page should probably reflect that. For example, --ep-admin-max-width would be --ep-admin-sync-card-max-width.

@mehidi258
Copy link
Contributor

@mehidi258 Yeah I agree with @felipeelia, I think it makes sense to put this into a shared config file that's imported into the other admin files too. With that usage in mind I'm thinking it makes sense to adjust some of the variable names. For example, I don't think we need the -01 on the color names, and some of the variables that are specific to the sync page should probably reflect that. For example, --ep-admin-max-width would be --ep-admin-sync-card-max-width.

@JakePT @felipeelia I have added vars.css file inside config and import it. Please review.

@felipeelia
Copy link
Member

Thanks, @mehidi258! @JakePT do you mind giving a look at the changes? I wonder if we wouldn't be better served with two vars files, one for the admin and another one for autosuggest and instant search (and any future frontend feature we create in the future). Thoughts? As is, this PR already looks like a step forward, just checking if we should go even further with the change. Thanks!

@JakePT
Copy link
Contributor

JakePT commented Feb 9, 2022

@felipeelia Yeah I think that's a good idea. Just one for admin and one for frontend, then we're not needlessly exposing admin custom properties on the front end and vice versa. I can take a crack at it this week if @mehidi258 is no longer available.

@mehidi258
Copy link
Contributor

@JakePT Added separate admin and frontend vars file. But there may need to refactor css files for using/composing that vars.
As we discussed on the call you can take this PR from now, Thanks.

@JakePT
Copy link
Contributor

JakePT commented Feb 24, 2022

@felipeelia As per our Slack discussion I have reverted this PR back to defining vars at the top of each feature file. In this case the PR is still just using vars for the sync page. I had started on setting up vars for the other features, and I was going to do a pass on the variables to clean them up a bit, but frankly I think a refactor of the markup and CSS across the admin might be necessary to get a cleaner set of vars, so I'm wondering if it might be better to leave this particular change for now (unless it's necessary to deal with linting) and revisit it as part of a larger clean up/refactor of the admin CSS. What do you think?

@felipeelia felipeelia merged commit cbc63bc into 4.x.x Feb 24, 2022
@felipeelia felipeelia deleted the chore/update-to-use-css-vars-issue-2498 branch February 24, 2022 12:20
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