Skip to content

Conversation

@jonkafton
Copy link
Contributor

@jonkafton jonkafton commented Sep 27, 2024

What are the relevant tickets?

Description (What does it do?)

Upgrades to latest course-search-utils with expend icon fixes and centers the close icon in the search utils input.

The close positioning issue is in the main branch as well, but this change depends on mitodl/course-search-utils#131, which replaces the Material <i /> icons with Remixicon SVGs.

Currently in Prod:

image

With fix:

image

and hover:

image

How can this be tested?

Yarn install and run. Close icon should appear as above.

This issue with the facet search inputs visbile when closed should also be fixed:
Before:
image
After:
image

Additional Context

We need to update the @mitodl/course-search-utils version once it's publish (it's currently pointing to the PR branch)

@jonkafton jonkafton added the Needs Review An open Pull Request that is ready for review label Sep 27, 2024
@jonkafton jonkafton marked this pull request as draft September 30, 2024 16:44
@jonkafton
Copy link
Contributor Author

Setting to draft until mitodl/course-search-utils#131 is published and version updated here.

@jonkafton jonkafton marked this pull request as ready for review October 9, 2024 17:32

type Colors = {
[Severity in AlertColor]: string
[_Severity in AlertColor]: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity, what was this for?

Same question below for _key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If was seeing an eslint error on the mapped type - a bug perhaps:

'Severity' is defined but never used. Allowed unused vars must match /^_/u.eslint[@typescript-eslint/no-unused-vars](https://typescript-eslint.io/rules/no-unused-vars)

top: 50%;
transform: translateY(-50%);
right: 7px;
top: 9px;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks/works fine, so 👍

However, pixels for vertical centering always seem fragile to me. The reason this wasn't working as expected before is shown below

Screenshot 2024-10-10 at 10 38 14 AM Screenshot 2024-10-10 at 10 40 33 AM

The issues were:

  • input had margin, which affected the content-box height of wrapper. If the margin goes on the wrapper instead, then top: 50%; transform: translate(-50%) works as expected for centering the button
  • The button is slightly larger than the SVG.

Anyway: If you want to use the percentage approach, (1) put margin on wrapper, (2) make sure button is same size as SVG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed the change to center at 50% - I agree the positioning shouldn't be susceptible to anything that might change the container height.

Also, the facet heading were not centered, for the same reason you pointed out above, in this case the chevron svg was displayed inline. It will be due to line-height being accounted for for inline elements, though as always with css the exact rules are byzantine to me!

@jonkafton jonkafton merged commit e0efafd into nextjs Oct 10, 2024
12 checks passed
@odlbot odlbot mentioned this pull request Oct 22, 2024
74 tasks
@rhysyngsun rhysyngsun deleted the jk/5472-embedded-icons branch February 7, 2025 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants