-
Notifications
You must be signed in to change notification settings - Fork 11
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: ds-783 update title SVG assets #468
Conversation
Relates to JIRA: DISCOVERY-783
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor increase in the size of the header, no one will notice. Appears to display and scale as intended, lgtm
Optional, non-blocking
-
Drop the commit that removes the currently unused SVG files. We may be restoring one of the empty SVG files to work within the limitations of the PF about modal, similar to what we did in earlier versions of the UI, like 1.8
-
The frustration with SVG and JSX is generally understood. Rework the last bit of copy for the commit message, it could be misconstrued. JSX/TSX can be easily confused with using regular HTML/XML. The current solution works within the limitations of the framework, but we encourage you to research working alternatives (like old-skool JS). And consider posting a PR to update the related UI code
Long explanation ahead. TLDR: I had to make SVG files in a very particular way to prevent patternfly/react/webpack from crashing on them. `titleBrand.svg` and `title.svg` were created using the `Red Hat Display` font in Inkscape which is, at the time of this writing, the official font for signage like this. Text objects were converted to paths before saving. The path objects produced for the text (e.g. "Discovery") were also broken into separate objects using the "Break apart" command. All objects include a faint hairline stroke (`rgba(00000033)`) so the white shapes are still visible on all-white backgrounds. The files were saved as "Plain SVG". These instructions are **VERY IMPORTANT** to follow precisely if you want to make future changes to SVGs in quipucords-ui like this. **DO NOT SAVE AS "Optimized SVG" OR POST-PROCESS THESE FILES THROUGH `scour`.** Although optimized files *are* perfectly valid and display correctly in all modern web browsers like Firefox, Chrome, and Safari as well as other popular SVG viewers, there are problems in how quipucords-ui invokes patternfly/react/webpack to "import" those SVG files (instead of simply copying them as static assets and referring to their paths in `src`/`srcset` attributes) that result in the UI failing to display them in the generated output. When "Quipucords" (`title.svg`) is saved as "Plain SVG", patternfly/react/webpack produces the following DOM, which correctly displays the image in the web browser, as seen by Firefox's inspector: <picture class="pf-v5-c-brand pf-m-picture" style="--pf-v5-c-brand--Height: 36px;"><source srcset="/images/4ca682d556b7bf59f5c6.svg"><img src="" alt="Quipucords logo"></picture> However, when that same SVG is "optimized", patternfly/react/webpack produces a `srcset` attribute containing the full contents of the file. This is very bad and wrong, and the web browser does not display the image. Here is an example of the DOM produced in that case, with the much longer `srcset` shortened for sake of display here: <picture class="pf-v5-c-brand pf-m-picture" style="--pf-v5-c-brand--Height: 36px;"><source srcset="data:image/svg+xml,%3c%3fxml version='1.0' encoding='UTF-8'%3f%3e %3c ... aria-label='Quipucords'/%3e%3c/g%3e%3c/svg%3e"><img src="" alt="Quipucords logo"></picture> When saving the files, why must we "Break apart" the path? This is to work around a similar issue in how the patternfly/react/webpack wrongly interprets or packs the file data. Some shapes cause patternfly/react/webpack parsing to fail and produce a `srcset` containing the full file just like it does when the SVG file is "optimized". For example, when the word "Discovery" (`titleBrand.svg`) starts with the capital letter `D` set in the `Red Hat Display` font and is a saved as a single shape object, patternfly/react/webpack fails to process that file. However, if you change that `D` to a different font *or* force the single path object (containing all letters' shapes) into separate path objects, patternfly/react/webpack seems to be okay with that, and it generates a working `srcset` attribute. The current solution works within the limitations of the framework, but we encourage you to research working alternatives (like old-school JS) and update the related UI code.
fc30122
to
0ce5efb
Compare
I kept the "delete" commit because the logo images are completely empty. They're literally just I did, however, amend the latter "fix" commit message to remove the last paragraph I wrote and instead paraphrase what David described in his comment above. |
What's included
Long explanation ahead. TLDR: I had to make SVG files in a very particular way to prevent patternfly/react/webpack from crashing on them.
titleBrand.svg
andtitle.svg
were created using theRed Hat Display
font in Inkscape which is, at the time of this writing, the official font for signage like this. Text objects were converted to paths before saving. The path objects produced for the text (e.g. "Discovery") were also broken into separate objects using the "Break apart" command. All objects include a faint hairline stroke (rgba(00000033)
) so the white shapes are still visible on all-white backgrounds. The files were saved as "Plain SVG".These instructions are VERY IMPORTANT to follow precisely if you want to make future changes to SVGs in quipucords-ui like this.
DO NOT SAVE AS "Optimized SVG" OR POST-PROCESS THESE FILES THROUGH
scour
. Although optimized files are perfectly valid and display correctly in all modern web browsers like Firefox, Chrome, and Safari as well as other popular SVG viewers, there are problems in how quipucords-ui invokes patternfly/react/webpack to "import" those SVG files (instead of simply copying them as static assets and referring to their paths insrc
/srcset
attributes) that result in the UI failing to display them in the generated output.When "Quipucords" (
title.svg
) is saved as "Plain SVG", patternfly/react/webpack produces the following DOM, which correctly displays the image in the web browser, as seen by Firefox's inspector:However, when that same SVG is "optimized", patternfly/react/webpack produces a
srcset
attribute containing the full contents of the file. This is very bad and wrong, and the web browser does not display the image. Here is an example of the DOM produced in that case, with the much longersrcset
shortened for sake of display here:When saving the files, why must we "Break apart" the path? This is to work around a similar issue in how the patternfly/react/webpack wrongly interprets or packs the file data. Some shapes cause patternfly/react/webpack parsing to fail and produce a
srcset
containing the full file just like it does when the SVG file is "optimized". For example, when the word "Discovery" (titleBrand.svg
) starts with the capital letterD
set in theRed Hat Display
font and is a saved as a single shape object, patternfly/react/webpack fails to process that file. However, if you change thatD
to a different font or force the single path object (containing all letters' shapes) into separate path objects, patternfly/react/webpack seems to be okay with that, and it generates a workingsrcset
attribute.I would implore quipucords-ui maintainers to coerce patternfly/react/webpack code to STOP trying to interpret SVG file data, and instead simply treat the files as static assets and pass their paths to the
srcset
orsrc
attributes. I don't know exactly what layer of the patternfly/react/webpack JavaScript stack is responsible for this bad behavior or if it's the fault of some other package they depend on; that's why I just keep saying "patternfly/react/webpack" collectively here. If our UI could simply treat static assets like SVGs as static assets instead of allowing JavaScript code mangle them, future edits should be much easier.How to test
Check the build with branding off and on. Confirm that the "Quipucords" and "Discovery" images appear in the header when
UI_BRAND
isfalse
andtrue
respectively.UI_BRAND=false npm start
UI_BRAND=true npm start