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

add protocol for prediction_context display #49

Merged
merged 4 commits into from
Nov 4, 2022

Conversation

leonlnj
Copy link
Contributor

@leonlnj leonlnj commented Oct 28, 2022

What this PR does / why we need it:
UI enhancement to support UPI routers in Turing. Most code changes are due to formatting.

The only main change is from / xp/ui/src/turing/components/form/EditExperimentEngineConfig.js whereby protocol is passed from Turing, all the way to xp/ui/src/turing/components/form/variables_config/components/FieldSourceFormLabel.js.

The value of Prediction Context stays the same as payload, as XP only supports HTTP, it is the Turing router that parses the UPI proto and send it to XP. This is mainly for visual aid to be consistency with traffic rules which shows Prediction Context too.

If no protocol is passed in, the original Payload will be shown.

image

@leonlnj leonlnj requested a review from a team October 28, 2022 10:13
Copy link
Member

@pradithya pradithya left a comment

Choose a reason for hiding this comment

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

whereby protocol is passed from Turing, all the way to xp/ui/src/turing/components/form/variables_config/components/FieldSourceFormLabel.js.

I wonder if using context will be good here. It seems a bit overkill though.

Other than that, LGTM!

@leonlnj leonlnj self-assigned this Oct 29, 2022
Copy link
Contributor

@terryyylim terryyylim 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 @leonlnj ! Curious - how did you generate these formatting changes? Are we not capturing them correctly in pre-commit linting step?

Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

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

Left one comment. The rest LGTM. Thanks!

@leonlnj
Copy link
Contributor Author

leonlnj commented Nov 2, 2022

Curious - how did you generate these formatting changes? Are we not capturing them correctly in pre-commit linting step?

I was using vs code extension prettier which format on save (if it doesn't work i write prettier cli manually)

@leonlnj leonlnj marked this pull request as ready for review November 2, 2022 09:50
};

export const capitalizeFirstLetter = (string) => {
return string.replace(/(^\w|\s\w)/g, (m) => m.toUpperCase());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than the custom function, we could use this: https://lodash.com/docs#startCase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wow thanks TIL. I was looking for ways to do with css/classname but just couldn't get all of it right with the nested context menu panel/item/dropdown. Ended up wit this custom func, didnt know lodash has such nifty one already.

@leonlnj leonlnj merged commit d3ff430 into caraml-dev:main Nov 4, 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.

None yet

4 participants