logo as image#6661
Conversation
|
# Conflicts: # storybooks/angular-storybook/CHANGELOG.md
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
# Conflicts: # .config/.jscpd.json # pnpm-lock.yaml
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 49 changed files in this pull request and generated 6 comments.
Files not reviewed (3)
- packages/vite-plugin/test/fixtures/react-app/package-lock.json: Language not supported
- packages/vite-plugin/test/fixtures/vue-app/package-lock.json: Language not supported
- pnpm-lock.yaml: Language not supported
# Conflicts: # __snapshots__/brand/showcase/chromium-highContrast/DBBrand-should-match-screenshot-1/DBBrand-should-match-screenshot.png # __snapshots__/brand/showcase/chromium/DBBrand-should-match-screenshot-1/DBBrand-should-match-screenshot.png # __snapshots__/brand/showcase/firefox/DBBrand-should-match-screenshot-1/DBBrand-should-match-screenshot.png # __snapshots__/brand/showcase/mobile-chrome/DBBrand-should-match-screenshot-1/DBBrand-should-match-screenshot.png # __snapshots__/brand/showcase/mobile-safari/DBBrand-should-match-screenshot-1/DBBrand-should-match-screenshot.png # __snapshots__/brand/showcase/webkit/DBBrand-should-match-screenshot-1/DBBrand-should-match-screenshot.png # __snapshots__/header/showcase/chromium-highContrast/DBHeader-should-match-screenshot-1/DBHeader-should-match-screenshot.png # __snapshots__/header/showcase/chromium/DBHeader-should-match-screenshot-1/DBHeader-should-match-screenshot.png # __snapshots__/header/showcase/firefox/DBHeader-should-match-screenshot-1/DBHeader-should-match-screenshot.png # __snapshots__/header/showcase/mobile-chrome/DBHeader-should-match-screenshot-1/DBHeader-should-match-screenshot.png # packages/components/package.json # pnpm-lock.yaml # scripts/package.json
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| IconProps & | ||
| ShowIconProps & |
There was a problem hiding this comment.
| IconProps & | |
| ShowIconProps & |
no more necessary
| } | ||
|
|
||
| &[data-no-text="true"] { | ||
| font-size: 0; |
There was a problem hiding this comment.
This might be an a11y issue. Screenreaders might skip texts with font-size 0.
"Do not size content to 0 pixels if you want the content to be read by a screen reader." (webaim)
There was a problem hiding this comment.
This might be an a11y issue. Screenreaders might skip texts with font-size 0.
"Do not size content to 0 pixels if you want the content to be read by a screen reader." (webaim)
I even also already came to that thought, it's good that you remind me of this. We should check our codebase in general, as we're already using it at other places, and want to add the label by aria-label additionally.
| "dev:html": "pnpm run copy-assets && pnpm run build-assets && pnpm run build-style:01_sass && vite --open", | ||
| "dev:react": "nodemon --watch src --watch scripts --watch configs --ext tsx,ts,cjs --exec \"pnpm run compile:react\"", | ||
| "dev:scss": "sass src:build --load-path=node_modules --watch --source-map", | ||
| "dev:scss": "sass src:build --load-path=node_modules --watch", |
There was a problem hiding this comment.
Shouldn't we keep the --source-map flag for dev?
| <DBBrand>(Default) With Logo</DBBrand> | ||
| <DBBrand hideLogo={true}>No Logo</DBBrand> | ||
| <DBBrand hideLogo={true}> | ||
| <DBBrand data-logo="db-systel" noText> |
There was a problem hiding this comment.
You added data-logo, but there is no CSS selector in brand.scss that overrides the --db-logo-url. Right now, this example will just render the default DB logo, right? I think we need to add the CSS logic and type the prop.
| content: ""; | ||
| block-size: #{variables.$db-sizing-sm}; | ||
| aspect-ratio: var(--db-logo-aspect-ratio); | ||
| background-image: var(--db-logo-url); | ||
| background-repeat: no-repeat; | ||
| background-size: auto #{variables.$db-sizing-sm}; |
There was a problem hiding this comment.
Logo became $db-sizing-sm from $db-sizing-md. Is this intended?
Proposed changes
Changed DBBrand component to render the default logo via CSS background images (served from foundations assets) rather than via the icon system
resolves https://github.com/db-ux-design-system/core-team/issues/2455
Types of changes
Further comments
🔭🐙🐈 Test this branch here: https://design-system.deutschebahn.com/core-web/review/test-logo-as-image