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

Replace holder.js with inline SVG #27633

Merged
merged 7 commits into from
Nov 20, 2018
Merged

Conversation

MartijnCuppens
Copy link
Member

@MartijnCuppens MartijnCuppens commented Nov 8, 2018

Replaces holder.js with a non-JavaScript solution.

Holder.js is still used on the example pages, that's why I didn't remove it yet. Merging #27605 could could make it easier to remove.

Wow, now I see @m5o did a similar PR a few hours ago

TODO:

  • Use this in examples too
  • Make highlight code not show the svg but instead an img tag
  • Always add the <title>?
  • Remove holder.js when everything else is ready
  • Maybe find another way so that we don't include docs.min.css in examples
  • Do we even need viewBox?
  • Make sure text is big in smaller screens too
  • Clean up/squash some patches before merging

Closes #27632 by superseding it and fixes #23185 along the way.

@MartijnCuppens MartijnCuppens requested a review from a team as a code owner November 8, 2018 17:49
@m5o

This comment has been minimized.

@MartijnCuppens

This comment has been minimized.

@XhmikosR

This comment has been minimized.

@XhmikosR XhmikosR reopened this Nov 12, 2018
@XhmikosR XhmikosR changed the title V4 dev martijncuppens placeholders Replace holder.js with inline SVG Nov 12, 2018
@XhmikosR XhmikosR force-pushed the v4-dev-martijncuppens-placeholders branch 2 times, most recently from 46828f6 to d90387f Compare November 13, 2018 13:20
@XhmikosR XhmikosR force-pushed the v4-dev-martijncuppens-placeholders branch 5 times, most recently from 87af4a6 to 4ddcaec Compare November 14, 2018 13:57
@MartijnCuppens MartijnCuppens requested a review from a team as a code owner November 14, 2018 14:00
@XhmikosR

This comment has been minimized.

@m5o

This comment has been minimized.

@XhmikosR XhmikosR force-pushed the v4-dev-martijncuppens-placeholders branch 2 times, most recently from 8149d62 to 7816fb2 Compare November 16, 2018 09:34
@XhmikosR
Copy link
Member

@m5o: currently it seems the alt is set to the text. Can we set it to the title instead?

@XhmikosR XhmikosR force-pushed the v4-dev-martijncuppens-placeholders branch from 92c9be6 to e759883 Compare November 16, 2018 10:19
@m5o
Copy link
Contributor

m5o commented Nov 16, 2018

@m5o: currently it seems the alt is set to the text. Can we set it to the title instead?

diff --git a/site/_includes/example.html b/site/_includes/example.html
index bdfda309f..3925b7c60 100644
--- a/site/_includes/example.html
+++ b/site/_includes/example.html
@@ -33,10 +33,10 @@ optional: hide_markup - disabled (default)
       {%- endif -%}
 
       {%- assign image_alt = include.content
-        | replace: '%">', '%">✂️'
-        | replace: '</text', '✂️</text'
+        | replace: '<title>', '<title>✂️'
+        | replace: '</title>', '✂️</title>'
         | split: '✂️' -%}
-      {%- assign image_alt = image_alt[1] | strip -%}
+      {%- assign image_alt = image_alt[1] -%}
 
       {%- for content_chunk in modified_content -%}
         {%- if content_chunk contains '<svg class="bd-placeholder-img' -%}
diff --git a/site/_includes/icons/placeholder.svg b/site/_includes/icons/placeholder.svg
index a4340c1d3..69d812a6c 100644
--- a/site/_includes/icons/placeholder.svg
+++ b/site/_includes/icons/placeholder.svg
@@ -28,7 +28,7 @@
 
 {%- capture svg -%}
 <svg class="bd-placeholder-img{% if class != '' %} {{ class }}{% endif %}" width="{{ width }}" height="{{ height }}" xmlns="http://www.w3.org/2000/svg"{% if viewBox != '' %} viewBox="{{ viewBox }}"{% endif %} preserveAspectRatio="xMidYMid slice">
-  {% if title != ' ' %}<title>{{ title }}</title>{% endif %}
+  <title>{{ title }}</title>
   <rect fill="{{ background }}" width="100%" height="100%"/>
   <text fill="{{ color }}" dy=".3em" x="50%" y="50%">{{ text }}</text>
 </svg>

💡 The if condition on the title is obsolete, if no title attribute is passed the default text 'Generic Placeholder Image' is used.

@XhmikosR
Copy link
Member

XhmikosR commented Nov 16, 2018

The condition might be useless now, but we didn't start with always adding the title :P

Thanks for the rest, I will update the patch.

EDIT:
Actually, now I remember why I had that check there. Because default doesn't apply when we pass a string with a space, thus this is a way to be able to skip the title in some cases.

@XhmikosR XhmikosR force-pushed the v4-dev-martijncuppens-placeholders branch 2 times, most recently from d8133a3 to 567c322 Compare November 16, 2018 16:02
Copy link
Member Author

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

Don't think the docs.min.css should be generated in this PR?

site/_includes/docs-sidebar.html Show resolved Hide resolved
site/_layouts/examples.html Outdated Show resolved Hide resolved
@XhmikosR

This comment has been minimized.

@XhmikosR

This comment has been minimized.

@m5o

This comment has been minimized.

@XhmikosR XhmikosR force-pushed the v4-dev-martijncuppens-placeholders branch from 1bcd254 to 26ac924 Compare November 19, 2018 09:50
@XhmikosR

This comment has been minimized.

@m5o

This comment has been minimized.

@XhmikosR XhmikosR force-pushed the v4-dev-martijncuppens-placeholders branch from 26ac924 to 313925f Compare November 19, 2018 10:39
@XhmikosR
Copy link
Member

Woot, thanks! Seems to be fine so far.

@XhmikosR XhmikosR force-pushed the v4-dev-martijncuppens-placeholders branch from 313925f to 5279c0b Compare November 19, 2018 10:43
@XhmikosR XhmikosR force-pushed the v4-dev-martijncuppens-placeholders branch 3 times, most recently from 2461fbb to 0d94436 Compare November 20, 2018 17:18
Copy link
Member Author

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

✅I've just tested this with all the changes, looks good!

XhmikosR and others added 6 commits November 20, 2018 21:24
* always include the title
* make it possible to skip adding the title by passing `title=' '`
* remove viewBox since we don't need it
@XhmikosR XhmikosR force-pushed the v4-dev-martijncuppens-placeholders branch from 0d94436 to 0e72b59 Compare November 20, 2018 19:25
@XhmikosR
Copy link
Member

Thanks @m5o for all the help with this, I really appreciate it <3

@XhmikosR XhmikosR merged commit 749c823 into v4-dev Nov 20, 2018
@XhmikosR XhmikosR deleted the v4-dev-martijncuppens-placeholders branch November 20, 2018 19:31
@mdo mdo mentioned this pull request Nov 20, 2018
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.

3 participants