Mwpw 162444 asset language #132
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## stage #132 +/- ##
==========================================
- Coverage 82.84% 82.62% -0.22%
==========================================
Files 10 10
Lines 3252 3303 +51
==========================================
+ Hits 2694 2729 +35
- Misses 558 574 +16 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
aem-code-sync
bot
temporarily deployed
to
MWPW-162444-asset-language
| @@ -205,9 +205,16 @@ export default class Search extends PartnerCards { | |||
| } | |||
|
|
|||
| generateFilters() { | |||
| function checkKey(key) { | |||
There was a problem hiding this comment.
@SonjaPopovic - regarding the below requirement from the ticket, I think we'll have to update that in the backend anyhow, so I don't think we will need this mapping from asset-language to language. I can simply change the key in the backend. Or do you need this for something else?
pages should not show up in the filtered view, even if they match the selected language.
There was a problem hiding this comment.
We can also update in on backend, i prepared pr for filtering only assets https://git.corp.adobe.com/wcms/dx-partners-runtime/pull/104 , but didn't want to change key in the backend since was not sure will that have side effects since search api is used on other pages. On logos pages only assets are fetched anyway, and on gnav search there is no language filter so i guess it is ok. I will prepare pr as part of this ticket. Thanks
There was a problem hiding this comment.
it shouldn't have any side effects, I think the fewer mappings we keep the better
| margin-right: 5px; | ||
| flex-shrink: 0; | ||
| } | ||
| .filter-info-text { |
There was a problem hiding this comment.
@SonjaPopovic the filter info text font looks a little wrong to me:
Can we just set a font size and line height? We could make use of the spectrum CSS variables provided:
.filter-info-text {
font-size: var(--type-body-xs-size);
line-height: 17px;
}
There was a problem hiding this comment.
I have different font, how are you testing this?
There was a problem hiding this comment.
It looks like there's somehow an issue for me when testing branches with the adobe clean font loading. Nevertheless, I would avoid redefining the font family in the info text and simply inherit it. The default should be:
font-family: var(--body-font-family);
| margin: 0 5px; | ||
| } | ||
|
|
||
| .info-icon { |
There was a problem hiding this comment.
@SonjaPopovic - I guess there's no spectrum web component that we can use out of the box for this?
|
|
||
| .filter-info { | ||
| display: flex; | ||
| margin: 0 5px; |
There was a problem hiding this comment.
@SonjaPopovic - there's no gap below the filter info to the first language in the list, looks a little odd when the item is hovered:
before: https://stage--dme-partners--adobecom.hlx.page/channelpartners/home/search/
after: https://MWPW-162444-asset-language--dme-partners--adobecom.hlx.page/channelpartners/home/search/?georouting=off