-
Notifications
You must be signed in to change notification settings - Fork 3
feat: make font family selectable #483
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
Conversation
|
oeninghe-dataport
left a comment
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.
src/core/utils/export/createMap.ts
Outdated
| if (typeof mapConfiguration.fontFamily !== 'undefined') { | ||
| void import('@kern-ux/native/dist/fonts/fira-sans.css') | ||
| } |
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.
If we move the import here, we require the usage of this function (which is not used in iceberg, for example).
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.
Maybe, a suitable option would be loading the font in loadKern using document.adoptedStyleSheets?
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.
I've got to move it, move it.
src/core/utils/loadKern.ts
Outdated
| kernSheet.replaceSync(` | ||
| @layer kern-ux { | ||
| ${kernCss.replaceAll(':root', ':host')} | ||
| ${typeof fontFamily !== 'undefined' ? `:host { --kern-typography-font-family-default: ${fontFamily}; }` : ''} |
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.
Why is this a separate parameter? Couldn't this be done using theme? (theme allows overriding arbitrary KERN variables, although the type probably needs to be adjusted.)
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.
I've got to move it, move it.
fa02f29 to
1484d11
Compare
1484d11 to
d7a5170
Compare
|
🏓 @oeninghe-dataport I have ... MOVED IT. |
oeninghe-dataport
left a comment
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.
Summary
Allow
fontFamilyconfiguration for map client.Instructions for local reproduction and review
I've
addedmodifieda snippetthe dataportTheme in the snowbox that can be uncommented to check the results in it. I suggest trying'Consolas', since it's easily distinguishable, and'', since it has a special meaning. When using'', set e.g.font-family: Book Antiqua;on the POLAR host node in the DOM manipulator to check whether it works as expected.I'm currently having an issue with vite and couldn't check the build. Please verify that the fonts are bundled separately to the other .css files so that they can only optionally be imported during run-time.
Pull Request Checklist (for Assignee)
Safari(Please verify in Safari!)UI has been tested in the following tools regarding accessibility (only regarding functionality affected in this PR)
Relevant tickets, issues, et cetera