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.

Sign up for GitHub

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

Jump to bottom

Update to the new version of the CreateObject component #426

Merged
merged 5 commits into from
Nov 1, 2019
Merged

Update to the new version of the CreateObject component #426

merged 5 commits into from
Nov 1, 2019

Conversation

adecler
Copy link
Member

@adecler adecler commented Oct 23, 2019

NOTE: Depends on

BHoM/BHoM_UI#147

Issues addressed by this PR

See BHoM/BHoM_UI#147 for more details

@adecler adecler added status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge type:feature New capability or enhancement labels Oct 23, 2019
@adecler adecler added this to the BHoM 3.0 β MVP milestone Oct 23, 2019
@adecler adecler self-assigned this Oct 23, 2019
@epignatelli
Copy link
Member

Quoting the other comment BHoM/BHoM_UI#147 (comment)

@epignatelli epignatelli added the status:WIP PR in progress and still in draft, not ready for formal review label Oct 23, 2019
@adecler adecler changed the title Expose the new CreateObject component Update to the new version of the CreateObject component Oct 29, 2019
@adecler adecler removed the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Oct 29, 2019
@adecler adecler marked this pull request as ready for review October 29, 2019 06:39
@adecler
Copy link
Member Author

adecler commented Oct 29, 2019

This is now ready for review

@adecler adecler removed the status:WIP PR in progress and still in draft, not ready for formal review label Oct 29, 2019
epignatelli
epignatelli reviewed Oct 31, 2019
View reviewed changes
Grasshopper_UI/Components/oM/CreateObject.cs

namespace BH.UI.Grasshopper.Components
{
public class CreateObjectComponent : CallerComponent
public class CreateObjectComponent : CallerComponent, IGH_VariableParameterComponent
Copy link
Member

Choose a reason for hiding this comment

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

I think there is no need to inherit from IGH_VariableParameterComponent.
CallerComponent already does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm weird, I remember having to add this after having problems with the inputs not updating correctly. But you're right it shouldn't need to be there. It doesn't do any damage either way so I guess its fine :-p

epignatelli
epignatelli reviewed Oct 31, 2019
View reviewed changes
Copy link
Member

@epignatelli epignatelli left a comment

Choose a reason for hiding this comment

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

Tested together with https://github.com/BHoM/BHoM_UI/pull/147/filesa and same feelings!

Some comments on the code side

Grasshopper_UI/Components/oM/CreateObject.cs
/**** Override Methods ****/
/*******************************************/

public override bool CanRemoveParameter(GH_ParameterSide side, int index)
Copy link
Member

Choose a reason for hiding this comment

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

Very much related to https://github.com/BHoM/BHoM_UI/pull/147/files#r341051725 for the missing CanCreateParameter.

It may be annoying from a UX perspective to erroneously remove an input, realising it after a series of operation, and could not CTRL + Z anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we discussed the idea of adding a 'reset' button in the context menu but I like your idea of the full list in the context menu better. Especially when considering the fact that it allows us to not have the base properties by default (except Name I guess)

@epignatelli
Copy link
Member

Approving in the light of BHoM/BHoM_UI#147 (comment)

Please, do not merge this.

@epignatelli epignatelli added the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Oct 31, 2019
@adecler
Copy link
Member Author

adecler commented Nov 1, 2019

@epignatelli , not a big deal as we will discus soon anyway but not actually approved :-p

@epignatelli epignatelli self-requested a review November 1, 2019 09:33
@epignatelli epignatelli removed the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Nov 1, 2019
@epignatelli epignatelli merged commit 934d134 into master Nov 1, 2019
@epignatelli epignatelli deleted the BHoM_UI-#124-NewCreateObjectComponent branch November 1, 2019 09:39
This was referenced Dec 19, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epignatelli epignatelli epignatelli approved these changes

@alelom alelom Awaiting requested review from alelom

@al-fisher al-fisher Awaiting requested review from al-fisher

@IsakNaslundBh IsakNaslundBh Awaiting requested review from IsakNaslundBh

Assignees

@adecler adecler

Labels
type:feature New capability or enhancement
Projects
None yet
Milestone
BHoM 3.0 β MVP
Development

Successfully merging this pull request may close these issues.
2 participants
@adecler @epignatelli