-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix GetElementScope method to work with multiple fragments #267
Fix GetElementScope method to work with multiple fragments #267
Conversation
@BHoMBot check required |
@michaelhoehn to confirm, the following checks are now queued:
|
@BHoMBot check installer |
@michaelhoehn to confirm, the following checks are now queued:
|
@BHoMBot check project-compliance |
@michaelhoehn to confirm, the following checks are now queued:
|
@BHoMBot check compliance |
@michaelhoehn to confirm, the following checks are now queued:
|
The check |
The check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a pure code review, I have requested changes to what is written in the file due to some redundant measures.
However, it is difficult to test/review this PR against the issue which was originally raised. The original issue is inadequate in providing replicable details for the bug, the steps necessary to reproduce and test independently, or even describing what should be expected vs should not be expected. As such, it will make it difficult to merge this PR when it is difficult to work out what the original problem was from one single screenshot that is inadequately described.
@FraserGreenroyd I tried to address your valid concerns regarding the issue description as well as adjust the code. Please feel free to re-review when you're able. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a code review this looks fine based on the conversations with @michaelhoehn
Thanks for the code review @FraserGreenroyd. I'm happy to merge this one I receive a functionality review from @kayleighhoude @enarhi or @shivanierambaran if possible. 🦖 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@BHoMBot check compliance |
@michaelhoehn to confirm, the following checks are now queued:
|
@BHoMBot check required |
@michaelhoehn to confirm, the following checks are now queued:
|
The check |
The check |
@michaelhoehn to confirm, the following checks are now queued:
There are 9 requests in the queue ahead of you. |
@BHoMBot check versioning |
@michaelhoehn to confirm, the following checks are now queued:
|
Dismissing as comments have been addressed.
@BHoMBot check ready-to-merge |
@michaelhoehn to confirm, the following checks are now queued:
There are 72 requests in the queue ahead of you. |
NOTE: Depends on
Issues addressed by this PR
Closes #266
Test files
Test File
Changelog
Additional comments