IBX-9069: Initial Product Tab #1397
Conversation
tischsoic
changed the base branch from
4.6
to
IBX-9227-multipleItemsLimit-not-respected
tischsoic
force-pushed
the
initial-product-picker
branch
from
f14fb1d
to
e36a086
Compare
| const Translator = getTranslator(); | ||
| const adminUiConfig = getAdminUiConfig(); | ||
| const itemsComponentsMap = useMemo(() => { | ||
| const { universalSelectItemsComponentsConfigs } = adminUiConfig.universalDiscoveryWidget; |
There was a problem hiding this comment.
Note: We already have selectedItemActions config for selected locations, so I've chosen universalSelectItemsComponentsConfigs, but I'm not happy with this name. Better name suggestions are welcome.
| box-shadow: calculateRem(4px) calculateRem(22px) calculateRem(47px) 0 rgba($ibexa-color-info, 0.05); | ||
|
|
||
| &::before, | ||
| &::after { |
There was a problem hiding this comment.
maybe we could do mixin from it? As I guess that's an arrow, already used in filters in two other places (also in theory arrow is also in dropdown, but in different direction, so probably unable to mixinise...)
| } | ||
|
|
||
| &__collapsible { | ||
| & + #{$filterRowCssClass} { |
There was a problem hiding this comment.
couldn't we achieve it by setting some constant classname that would just need to be used in html?
Just looking for other solution, current one is probably ok, just looks strange for me (probably needs to get used to passing classname as variable)
There was a problem hiding this comment.
I guess you cannot generate a CSS selector based on the CSS variable set on the DOM element?
| background: $ibexa-color-dark; | ||
| } | ||
|
|
||
| &::before { |
There was a problem hiding this comment.
oh hey, next arrow css code, so probably mixin makes sense now 100%. :)
| }; | ||
|
|
||
| FiltersPanel.propTypes = { | ||
| children: PropTypes.node.isRequired, |
There was a problem hiding this comment.
total nitpick, not sure if props like this should be required... depends how you look on it - this component would work without it 100% after all. But at the same time, probably wouldn't make sense in context we're using it in.
There was a problem hiding this comment.
I assumed this component would not make much sense without children, but on the other hand, I don't think such a requirement adds much value... I will delete it.
|
|
||
| FiltersRow.propTypes = { | ||
| children: PropTypes.node.isRequired, | ||
| extraClasses: PropTypes.string.isRequired, |
There was a problem hiding this comment.
I doubt extraClasses should be treated as required, I think it was always optional to pass this specific prop in our system?
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| parseTooltip(refSelectedLocationsItem.current); |
There was a problem hiding this comment.
I know that's totally not place for it, but just a hunch I want to mention - shouldn't we put hideAllTooltips in return function? As probably we should make sure we hide all tooltips when component is destroyed (marginally low chance for such case, as such component would need to cease to exist independently from user input, as user needs to hover cursor over tooltipable element)
There was a problem hiding this comment.
We should put it in the ref function, I copied this code from another place and forgot to refactor it. :)
| {selectedItems.map((selectedItem) => { | ||
| const ItemComponent = itemsComponentsMap[selectedItem.type].component; | ||
|
|
||
| return ItemComponent && <ItemComponent key={`${selectedItem.type}-${selectedItem.id}`} item={selectedItem} />; |
There was a problem hiding this comment.
is there a chance component wouldn't exist? and if there is, do we want to silent error the fact there's probably some misconfiguration and specific selected item doesn't show up?
| @@ -13,8 +15,19 @@ const SortSwitcher = ({ isDisabled }) => { | |||
| setSortOrder(option.sortOrder); | |||
| }; | |||
|
|
|||
| const disabledParams = {}; | |||
There was a problem hiding this comment.
There probably shouldn't be empty like above (line 15/17)?
| const isNotSelectable = | ||
| (containersOnly && !isContainer) || (allowedContentTypes && !allowedContentTypes.includes(contentTypeIdentifier)); | ||
| const isSelectionBlocked = multipleItemsLimit !== 0 && !isSelected && selectedLocations.length >= multipleItemsLimit; |
There was a problem hiding this comment.
you've created helper for it (src/bundle/ui-dev/src/modules/universal-discovery/helpers/selected.locations.helper.js) couldn't you use it here, or difference is structure?
There was a problem hiding this comment.
These items are not necessarily locations so I didn't want to use location-specific helpers.
|
|
||
| useEffect(() => { | ||
| dispatch({ type: FETCH_START }); | ||
| const offset = state.pageIndex * itemsPerPage; |
There was a problem hiding this comment.
| const offset = state.pageIndex * itemsPerPage; | |
| const offset = state.pageIndex * itemsPerPage; |
|
| & + .c-filters-row, | ||
| & + .c-filters__row { |
There was a problem hiding this comment.
something is wrong with this for me :) c-filters-row and c-filters__row should be the same component but now it's separate or am I wrong
There was a problem hiding this comment.
I created some new filters components and now we have c-filters-row and we no longer have c-filters__row, but because this is an add-on for 4.6 I wanted to be extra careful with BC break and didn't delete this CSS styles. But maybe I should not be that careful. :)
Related PRs:
Description:
For QA:
Documentation: