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

fix: removed the extra spacing between the chips in search and filter from vanilla example #5414

Merged

Conversation

Nisarg-18
Copy link
Contributor

@Nisarg-18 Nisarg-18 commented Nov 28, 2024

Done

Removed extra spacing between search & filter chips that are only present in the Vanilla examples by setting the font-size to 0 as suggested by @lyubomir-popov in the issue conversation.

Fixes #5227

QA

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix release (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

Screenshot

Screenshot 2024-11-28 at 2 55 02 AM

@webteam-app
Copy link

webteam-app commented Nov 28, 2024

@Nisarg-18 Nisarg-18 marked this pull request as ready for review November 28, 2024 07:28
@Nisarg-18 Nisarg-18 marked this pull request as draft November 28, 2024 07:29
@Nisarg-18 Nisarg-18 closed this Nov 28, 2024
@Nisarg-18 Nisarg-18 reopened this Nov 28, 2024
@Nisarg-18 Nisarg-18 marked this pull request as ready for review November 28, 2024 07:47
@bartaz
Copy link
Member

bartaz commented Dec 2, 2024

Thank you for you contribution.

Reducing font size to 0 may end up being quite risky change (as it may affect child elements if it's not reset back to standard size. It's probably not worth that risk.

Another, simpler and safer fix proposed in the issue was to simply remove the excess whitespace from the example HTML code. This component is used only in its React implementation, (where whitespaces between chips are not rendered), so it seems valid to reflect that in HTML of our examples.

Would you be willing to do this change instead @Nisarg-18 ?

@Nisarg-18
Copy link
Contributor Author

Yep, I agree, It is a risky change.

Yes, I will implement the safer option of changing the HTML code.

@Nisarg-18
Copy link
Contributor Author

@bartaz, I have made the changes in the HTML code. I have used comments (<!-- -->) instead of putting everything in a single line to make the code more readable. Let me know if this syntax is okay.

@jmuzina jmuzina added Review: QA needed Review: Code needed Review: Percy needed This PR needs a review of Percy for visual regressions Review: Percy +1 and removed Review: Percy needed This PR needs a review of Percy for visual regressions labels Dec 2, 2024
@bartaz
Copy link
Member

bartaz commented Dec 3, 2024

OK. I'd probably just do them all in one line, but it doesn't matter too much. As I said, this component is meant to be used via React anyway.

It does look a bit weird in code preview in docs, but it's not too bad:
image

Thank you for your contribution @Nisarg-18.

@bartaz bartaz merged commit 3e85ce0 into canonical:main Dec 3, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Space character causes unexpected spacing in Search & Filter examples
4 participants