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

Trim spaces and line breaks beside icons #2958

Merged
merged 26 commits into from
Dec 11, 2023

Conversation

precious-onyenaucheya-ons
Copy link
Contributor

@precious-onyenaucheya-ons precious-onyenaucheya-ons commented Nov 24, 2023

What is the context of this PR?

Fixes #2873
Currently there extra spaces beside icons in the Design System. Following the suggestion made in the issue linked, I made use of the -- syntax in Nunjucks to trim any trailing whitespace or line breaks.

I have also updated the external link component to use the onsIcon rather than <svg>. Tests to cover this scenario have been added as well.

How to review this PR

Check components which include an icon if there is an extra white space. Use the image in the issue linked to the PR as a reference

Checklist

This needs to be completed by the person raising the PR.

  • I have selected the correct Assignee
  • I have linked the correct Issue

Copy link

netlify bot commented Nov 24, 2023

Deploy Preview for ons-design-system-preview ready!

Name Link
🔨 Latest commit 07f611e
🔍 Latest deploy log https://app.netlify.com/sites/ons-design-system-preview/deploys/6560ace6d02da600087a8ea3
😎 Deploy Preview https://deploy-preview-2958--ons-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@precious-onyenaucheya-ons precious-onyenaucheya-ons added Bug Something isn't working Enhancement Change of existing feature labels Nov 24, 2023
@precious-onyenaucheya-ons precious-onyenaucheya-ons linked an issue Nov 24, 2023 that may be closed by this pull request
Copy link

netlify bot commented Nov 24, 2023

Deploy Preview for ons-design-system-preview ready!

Name Link
🔨 Latest commit d9a0c67
🔍 Latest deploy log https://app.netlify.com/sites/ons-design-system-preview/deploys/6572e81177f87c0007fd2240
😎 Deploy Preview https://deploy-preview-2958--ons-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@precious-onyenaucheya-ons precious-onyenaucheya-ons removed the Enhancement Change of existing feature label Nov 24, 2023
@precious-onyenaucheya-ons precious-onyenaucheya-ons changed the title minified html Minify html in DS Nov 24, 2023
@rmccar
Copy link
Contributor

rmccar commented Nov 29, 2023

The padding around the pagination component and breadcrumbs component now needs updating. As this issue only affects places where an SVG is placed after text and not before the padding is no longer consistent between the two.

Screenshot 2023-11-29 at 15 30 10

Screenshot 2023-11-29 at 15 30 30

I also think we need to run these changes past Dina to see if shes happy with the padding or prefers the padding before the change. If she prefers that latter than I still think we should fix this but just increase the actual margin.

Also we are no longer minifying the HTML, just removing line breaks before svg elements so the PR title and description should be updated.

There are other benefits to minification are we happy to not minify? We need to research and discuss this because we could easily have the minification you had before alongside the regex which could be the best of both.

@precious-onyenaucheya-ons precious-onyenaucheya-ons changed the title Minify html in DS Trim spaces and line breaks beside Icons Dec 1, 2023
@precious-onyenaucheya-ons precious-onyenaucheya-ons changed the title Trim spaces and line breaks beside Icons Trim spaces and line breaks beside icons Dec 1, 2023
@rmccar
Copy link
Contributor

rmccar commented Dec 4, 2023

In the Icon macro the dash syntax has been added in too many places, we only need the change before and after the icons. It can be removed from the classes.

Also the pagination component still has some space in it
Screenshot 2023-12-01 at 13 33 45

@rmccar
Copy link
Contributor

rmccar commented Dec 4, 2023

As part of this PR the external link component can now be updated to use the icon component instead of hardcoding the svg. The external link icon might need to be added to the icon macro to do that

src/components/icon/_macro.njk Show resolved Hide resolved
src/components/icon/_macro.njk Outdated Show resolved Hide resolved
src/components/external-link/_macro.njk Outdated Show resolved Hide resolved
src/components/external-link/_macro.njk Show resolved Hide resolved
Copy link
Contributor

@rmccar rmccar left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-12-06 at 09 47 47

The changes seem to have increased the space between the external link icon and the link itself

@precious-onyenaucheya-ons
Copy link
Contributor Author

Screenshot 2023-12-06 at 09 47 47

The changes seem to have increased the space between the external link icon and the link itself

This is because of the span that contains &nbsp; which was added. So I'd imagine the space is expected

@rmccar
Copy link
Contributor

rmccar commented Dec 6, 2023

Screenshot 2023-12-06 at 09 47 47
The changes seem to have increased the space between the external link icon and the link itself

This is because of the span that contains &nbsp; which was added. So I'd imagine the space is expected

Yes but the space has increased which is not what we want to happen. The &nbsp has always been there to stop the line breaking at that point. Ideally we want the space to stay the same including keeping the &nbsp but use the icon component. The other option is to look into other ways of stopping the line breaking at that point. But overall we don't want the space between the icon and text to increase

Copy link
Contributor

@rmccar rmccar left a comment

Choose a reason for hiding this comment

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

Couple of minor things

src/components/external-link/_macro.njk Outdated Show resolved Hide resolved
__snapshots__/layout/_template.spec.js.snap Show resolved Hide resolved
@precious-onyenaucheya-ons precious-onyenaucheya-ons merged commit cb14f4e into main Dec 11, 2023
9 checks passed
@precious-onyenaucheya-ons precious-onyenaucheya-ons deleted the feature/minify-html-in-DS branch December 11, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTML not being minified causing extra whitespace
3 participants