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

fix: control getShorthand returns array-valued property #170

Merged
merged 10 commits into from
Mar 25, 2022

Conversation

ph-alsvik
Copy link
Contributor

Very simple fix to when get$Shorthand-method returns a control prop that is an array of values (for example when calling getSelectedKeys on a MultiComboBox-control which returns an array of strings).

Please correct me on this, but AFAIK there is no way of knowing from the get$Shorthand what kind of return value is expected, other than calling the method and inspecting the returned value. This creates an edge case when the array is empty, because we don’t know if its an array of aggregates or js primitives. Therefore I’ve created a separate case “empty” for when the the returned value is an empty array, and warn the user that there is no data found in prop or aggregation.

Perhaps there is a more elegant way to address this issue?

@vobu vobu self-requested a review March 16, 2022 15:45
@vobu
Copy link
Contributor

vobu commented Mar 20, 2022

Hi @ph-alsvik - changes look good to me!
As you stated, I also don't see a way of "forecasting" the return value of a get$Shorthand UI5 control's method.
So your solution makes sense and we'd like to get it in.
Any chance you could add a dedicated test for Strings[] and empty in /examples/ui5-js-app/webapp/test/e2e/**/*.test.js?

1 similar comment
@vobu
Copy link
Contributor

vobu commented Mar 20, 2022

Hi @ph-alsvik - changes look good to me!
As you stated, I also don't see a way of "forecasting" the return value of a get$Shorthand UI5 control's method.
So your solution makes sense and we'd like to get it in.
Any chance you could add a dedicated test for Strings[] and empty in /examples/ui5-js-app/webapp/test/e2e/**/*.test.js?

@vobu
Copy link
Contributor

vobu commented Mar 20, 2022

ps: happy to blueprint a test case if needed.
and also would like to direct your attention to #191 - we'll tame that beast soon :)

1 similar comment
@vobu
Copy link
Contributor

vobu commented Mar 20, 2022

ps: happy to blueprint a test case if needed.
and also would like to direct your attention to #191 - we'll tame that beast soon :)

@ph-alsvik
Copy link
Contributor Author

Hi @vobu ! Sure, I'll make a test. Should have to add in the MultiComboBox control to the ui5-js app, right?

@vobu
Copy link
Contributor

vobu commented Mar 21, 2022

Hi @vobu ! Sure, I'll make a test. Should have to add in the MultiComboBox control to the ui5-js app, right?

yep, exactly! just put it any place, layout is not the primary concern here 🤣

@ph-alsvik
Copy link
Contributor Author

Hope this test covers it! Just let me know if I should alter it :)

@vobu
Copy link
Contributor

vobu commented Mar 24, 2022

Hope this test covers it! Just let me know if I should alter it :)

will do!
and thanks for the PR!
the tests will fail, as we're currently working on getting a semver comparisons right in oder to cope with UI5 1.100 -
i will let you know once our enhancements are in so you can re-merge the then most recent main into your PR

@vobu
Copy link
Contributor

vobu commented Mar 24, 2022

Hope this test covers it! Just let me know if I should alter it :)

will do! and thanks for the PR! the tests will fail, as we're currently working on getting a semver comparisons right in oder to cope with UI5 1.100 - i will let you know once our enhancements are in so you can re-merge the then most recent main into your PR

semver fix now in main → please merge main again into your PR and let'em tests run :)

@ph-alsvik
Copy link
Contributor Author

semver fix now in main → please merge main again into your PR and let'em tests run :)

Hi @vobu! Hope I did everything right (fairly new to Git).

@vobu
Copy link
Contributor

vobu commented Mar 25, 2022

semver fix now in main → please merge main again into your PR and let'em tests run :)

Hi @vobu! Hope I did everything right (fairly new to Git).

you're doing great then!
there are still merge conflicts in

  • client-side-js/executeControlMethod.js
  • examples/ui5-js-app/webapp/controller/Main.controller.js
  • examples/ui5-js-app/webapp/model/countries.json
  • examples/ui5-js-app/webapp/view/Main.view.xml

Please look through them and make sure, none of the recent changes in main break your enhancements.
Then once the tests go green, let's get your thingie in!!!

Copy link
Contributor

@vobu vobu left a comment

Choose a reason for hiding this comment

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

excellent 🤝

@vobu vobu merged commit b7a2789 into ui5-community:main Mar 25, 2022
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.

2 participants