Skip to content
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

IBX-9069: Initial Product Tab #1397

Merged
merged 16 commits into from
Dec 20, 2024
Merged

Conversation

tischsoic
Copy link
Contributor

@tischsoic tischsoic commented Nov 22, 2024

🎫 Issue IBX-9069

Related PRs:

Description:

For QA:

Documentation:

@tischsoic tischsoic self-assigned this Nov 22, 2024
@tischsoic tischsoic changed the base branch from 4.6 to IBX-9227-multipleItemsLimit-not-respected November 25, 2024 08:37
@tischsoic tischsoic force-pushed the initial-product-picker branch from f14fb1d to e36a086 Compare November 25, 2024 08:38
@tischsoic tischsoic marked this pull request as ready for review November 25, 2024 08:42
const Translator = getTranslator();
const adminUiConfig = getAdminUiConfig();
const itemsComponentsMap = useMemo(() => {
const { universalSelectItemsComponentsConfigs } = adminUiConfig.universalDiscoveryWidget;
Copy link
Contributor Author

@tischsoic tischsoic Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to come up with a proposal:
universalSelectionComponentsConfig - Barely shorter and not even sure if better

Base automatically changed from IBX-9227-multipleItemsLimit-not-respected to 4.6 November 25, 2024 13:11
}

&__collapsible {
& + #{$filterRowCssClass} {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you cannot generate a CSS selector based on the CSS variable set on the DOM element?

const isNotSelectable =
(containersOnly && !isContainer) || (allowedContentTypes && !allowedContentTypes.includes(contentTypeIdentifier));
const isSelectionBlocked = multipleItemsLimit !== 0 && !isSelected && selectedLocations.length >= multipleItemsLimit;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helpers from selected.locations.helper.js use location but in this component we have only access to location's id, which would be sufficient for selected.locations.helper.js, but these helpers were designed with location parameter and I don't want to change it now, as this can result in BC.

<div className="c-selected-items-panel__items-wrapper">
{renderActionBtns()}
<div className="c-selected-items-panel__items-list">
{selectedItems.map((selectedItem) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure which approach is more consistent with the overall design, but for me, the code is clearer if this logic would be moved to a separate function. What do you think about it?

@tischsoic tischsoic force-pushed the initial-product-picker branch from 8126e07 to af1e453 Compare December 18, 2024 10:21
@tischsoic tischsoic force-pushed the initial-product-picker branch from af1e453 to fbb9815 Compare December 20, 2024 09:07
@tischsoic tischsoic changed the base branch from 4.6 to feature-discount December 20, 2024 10:59
@tischsoic tischsoic merged commit ac4bae7 into feature-discount Dec 20, 2024
30 checks passed
@tischsoic tischsoic deleted the initial-product-picker branch December 20, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants