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

Sprint 201903 dynamic properties #35

Open
wants to merge 2 commits into
base: sprint-201903-editable-properties
Choose a base branch
from

Conversation

sabicalija
Copy link
Contributor

No description provided.

@sabicalija
Copy link
Contributor Author

One point is missing. When no ARE is running and WebACS is loading dynamic properties: the input field should load and display the property value, and not an empty input field instead.

@klues
Copy link
Contributor

klues commented Mar 12, 2019

why a PR for merging sprint-201903-editable-properties from sprint-201903-dynamic-properties? To my mind we only need PRs if the want to merge into master.

@deinhofer
Copy link
Contributor

why a PR for merging sprint-201903-editable-properties from sprint-201903-dynamic-properties? To my mind we only need PRs if the want to merge into master.

correct. Please change PR to merge into master

@sabicalija
Copy link
Contributor Author

sabicalija commented Mar 12, 2019

I did not start from master when creating this branch but from sprint-201903-editable-properties. If we change it to master now, the diff will contain changes from both, sprint-201903-editable-properties and sprint-201903-dynamic-properties.

What would be the correct process? Branch from master or is branching from other feature/bugfix branches also okay?

I can cherry-pick those two commits in a new branch, starting from master, in case we really need to change it.

@sabicalija sabicalija changed the base branch from sprint-201903-editable-properties to master March 12, 2019 17:50
@sabicalija sabicalija changed the base branch from master to sprint-201903-editable-properties March 12, 2019 17:50
@klues
Copy link
Contributor

klues commented Mar 13, 2019

In the current situation there are several possibilities:
(1) one PR sprint-201903-dynamic-properties into master containing all differences.
Pro: easiest
Con: PR's get big and not easy to overlook

(2)
git checkout master
git checkout -b sabic/issue#18/dynamic-properties
git checkout sprint-201903-dynamic-properties -- <folder or files that are necessary for this PR>
git commit ...
git push origin sabic/issue#18/dynamic-properties
and create a PR for this new branch sabic/issue#18/dynamic-properties into master only containing the changes that are needed for issue#18
Pro: smaller PR's, easy to overlook
Con: more complex

(3) same as (2), but using cherry-pick if you know the single commits you did for solving issue#18.

For my recent 6 PR's on the AsTeRICS repository I followed approach (2).

In general a simpler appraoch would have been:
a) start with master
b) create new branch sabic/issue#18/dynamic-properties
c) fix issue#18
d) create PR for branch sabic/issue#18/dynamic-properties into master
e) checkout sprint-201903-editable-properties and call git merge master (after PR merged) or git merge sabic/issue#18/dynamic-properties (before PR merged), if you need the new changes in your working branch.

@sabicalija sabicalija self-assigned this Mar 13, 2019
@sabicalija
Copy link
Contributor Author

I did it. 😄 Thanks for the detail explanation! #36

@sabicalija sabicalija closed this Mar 13, 2019
@sabicalija sabicalija deleted the sprint-201903-dynamic-properties branch March 14, 2019 20:16
@sabicalija sabicalija restored the sprint-201903-dynamic-properties branch May 9, 2019 23:48
@sabicalija
Copy link
Contributor Author

Dynamic properties are not displayed after the model is uploaded and to the ARE. Reload of page and download of the model is necessary to enable the display of the dynamic properties

@sabicalija sabicalija reopened this May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants