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

[ui] Fixes dynamic options #1050

Merged
merged 7 commits into from Sep 28, 2019
Merged

[ui] Fixes dynamic options #1050

merged 7 commits into from Sep 28, 2019

Conversation

lolodomo
Copy link
Contributor

Fix #1040

Signed-off-by: Laurent Garnier lg.hc@free.fr

cweitkamp and others added 3 commits September 28, 2019 17:22
@cweitkamp
Use fragment.options if own options are empty
9e16bc1 
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@cweitkamp
Added unit tests
b28204c 
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@lolodomo
Fix dynamic options in UI
4952118 
Fix #1040

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor Author

@cweitkamp : can you please review and check that your change has no other impact.
This is not clear for me if we are handling null or empty list for options.

@cweitkamp
Copy link
Contributor

@lolodomo Here you are: lolodomo#2

@cweitkamp cweitkamp added the bug An unexpected problem or unintended behavior of the Core label Sep 28, 2019
cweitkamp and others added 3 commits September 28, 2019 17:49
@cweitkamp
Added unit tests
0170835 
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@cweitkamp
Applied review comments
a22aa0d 
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@lolodomo
Merge pull request #2 from cweitkamp/bugfix-1040-merge-if-options-empty
caffc9d 
Use fragment.options if own options are empty -> Added unit tests
@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 28, 2019 •
edited

I merged your tests but did not test them.

cweitkamp
cweitkamp approved these changes Sep 28, 2019
View reviewed changes
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

The only thing I worry about is: What happens if the original fragment has options and the new fragment to be merged with too? So it is not really a "merge" but more or less a "use own unless null or empty than others".

@cweitkamp cweitkamp merged commit 7b49f27 into openhab:master Sep 28, 2019
@cweitkamp cweitkamp added this to the 2.5 milestone Sep 28, 2019
@lolodomo
Copy link
Contributor Author

Yes, that's true. But I don't know in practice how it could occur.

@lolodomo lolodomo deleted the dynamicOptions branch September 28, 2019 21:50
@cweitkamp cweitkamp added the UI User Interface label Dec 3, 2019
@cweitkamp cweitkamp changed the title Fix dynamic options in UI [ui] Fixes dynamic options Dec 3, 2019
@kaikreuzer kaikreuzer removed the bug An unexpected problem or unintended behavior of the Core label Dec 13, 2019
Rosi2143 pushed a commit to Rosi2143/openhab-core that referenced this pull request Dec 26, 2020
@cweitkamp @Confectrician
Update restdocs.md (openhab#1050)
96c0e79 
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
@lolodomo @splatch
Fix dynamic options in UI (openhab#1050)
698153c 
* Fix dynamic options in UI
* Added unit tests

Fixes openhab#1040

Also-by: Christoph Weitkamp <github@christophweitkamp.de>
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
GitOrigin-RevId: 7b49f27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cweitkamp cweitkamp cweitkamp approved these changes

Assignees
No one assigned
Labels
UI User Interface
Projects
None yet
Milestone
2.5
Development

Successfully merging this pull request may close these issues.

Dynamic state description broken in snapshot 1693
3 participants
@lolodomo @cweitkamp @kaikreuzer