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-2937: Added new relation starting location options #432

Merged
merged 2 commits into from
Jul 6, 2022

Conversation

Nattfarinn
Copy link
Contributor

@Nattfarinn Nattfarinn commented May 24, 2022

Question Answer
Tickets https://issues.ibexa.co/browse/IBX-2937
Bug fix? no
New feature? yes
BC breaks? no
Tests pass? yes
Doc needed? yes
License GPL-2.0

Screenshot 2022-05-27 at 12 14 54

Not a stand alone PR.

Requires:

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@Nattfarinn Nattfarinn requested review from a team and removed request for a team May 24, 2022 08:00
@Nattfarinn Nattfarinn changed the title IBX-2937: Added new relation starting location options (WIP) IBX-2937: Added new relation starting location options May 24, 2022
@juskora juskora added the Doc needed The changes require some documentation label May 25, 2022
@Nattfarinn Nattfarinn changed the title (WIP) IBX-2937: Added new relation starting location options IBX-2937: Added new relation starting location options May 27, 2022
@@ -303,7 +303,7 @@ const UniversalDiscoveryModule = (props) => {
}, [currentView]);

useEffect(() => {
if (!props.startingLocationId || props.startingLocationId === 1) {
if (!props.startingLocationId || props.startingLocationId === 1 || props.startingLocationId === props.rootLocationId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also update the dependencies of this hook (line :324).

Dependencies are responsible for establishing what properties will cause the useEffect hook to recalculate. In this case you're adding props.rootLocationId as part of this condition, so if it ever changes, it should cause useEffect to re-apply, just as props.startingLocationId does.

Although, from what I see, other dependencies are also missing (like currentView).

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

👍 for PHP code 😅

@Nattfarinn Nattfarinn requested a review from Steveb-p July 5, 2022 08:31
Copy link
Contributor

@mnocon mnocon left a comment

Choose a reason for hiding this comment

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

Tested all the PRs together on relation and relationlist fieldtypes, looks good!

QA Approved

@Nattfarinn Nattfarinn merged commit c8b6e23 into main Jul 6, 2022
@Nattfarinn Nattfarinn deleted the ibx-2937-relation-children branch July 6, 2022 14:00
@juskora juskora removed the Doc needed The changes require some documentation label Jul 22, 2022
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.

7 participants