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 selected facet component #36

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Add selected facet component #36

merged 1 commit into from
Sep 3, 2024

Conversation

jcoyne
Copy link
Collaborator

@jcoyne jcoyne commented Aug 27, 2024

Fixes #35

@dnoneill
Copy link
Contributor

@jcoyne Looks mostly good. When I tested this in earthworks I noticed the off center is back so I think we need to add the flex in.

I think this is happening because we are decreasing the padding-y and it is making the lack to centering more noticable. I tried in Chrome (version 128.0.6613.85, which says its up to date). So I think this might be an issue.

With --padding-y set to default
Screenshot 2024-08-27 at 5 39 32 PM

With --padding-x set to .25rem
Screenshot 2024-08-27 at 5 39 44 PM

@jcoyne
Copy link
Collaborator Author

jcoyne commented Aug 28, 2024

@dnoneill do you have a branch to share?

@dnoneill
Copy link
Contributor

@jcoyne I copied the CSS into earthworks.css on this branch sul-dlss/earthworks#1221

@jcoyne
Copy link
Collaborator Author

jcoyne commented Aug 30, 2024

@dnoneill I think the problem there is that Earthworks is putting it into a line that also has the "Start over" button. The start over button is pushing up the line height, so the selected facet component is growing to match that size (eliminating --bs-btn-padding-y on the facet, or adding it to the "start over" allows the font to center). I think we need to think of this more expansively. E.g. Do we want the "Start over" button to be taller than the selected facet buttons?

@dnoneill
Copy link
Contributor

@jcoyne I tested locally and you are right about the start over button causing the issue. I think this okay to merge and I can solve the problem in the constraints_component

@dnoneill
Copy link
Contributor

@jcoyne I opened #45 which solves the problem in earthworks. It also fixes the fact we don't have button padding defined correctly in the component library.

@jcoyne jcoyne merged commit 3c98ff4 into main Sep 3, 2024
1 check passed
@jcoyne jcoyne deleted the selected_facet branch September 3, 2024 13:56
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.

Implement Selected Facet
2 participants