Skip to content

Move deprecated AppUI functionality into mobile-ui-react.#75

Draft
tcobbs-bentley wants to merge 5 commits intomainfrom
travis/appui-deprecations-local-copies
Draft

Move deprecated AppUI functionality into mobile-ui-react.#75
tcobbs-bentley wants to merge 5 commits intomainfrom
travis/appui-deprecations-local-copies

Conversation

@tcobbs-bentley
Copy link
Member

No description provided.

@tcobbs-bentley tcobbs-bentley requested a review from a team as a code owner July 16, 2024 17:30
* deprecated from @itwin/mobile-ui-react.
* @public
*/
export interface ClassNameProps {
Copy link
Member

Choose a reason for hiding this comment

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

lets review with the appui team why this was deprecated, and what they expected consumers to do, before we re-export a deprecated type from another repo as public here

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this to draft for now while we decide what to do and created a new PR to allow lint to pass.

@tcobbs-bentley tcobbs-bentley marked this pull request as draft July 16, 2024 20:48
// @todo AppUI deprecation
// eslint-disable-next-line deprecation/deprecation
}, [handler]), SessionStateActionId.SetIModelConnection);
const iModelConnection = useActiveIModelConnection();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest removing this function since it doesn't add anything that can't be accomplished with useActiveIModelConnection

Copy link
Member Author

Choose a reason for hiding this comment

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

SYNCHRO Field uses it in one place, and they take advantage of the fact that they get a callback when it happens. They don't actually care about the value, simply that it changed:

  useIModel(() => {
    reset();
  });

Obviously they could update their code to React.useMemo like I do here. I guess we could ask them.

Copy link
Contributor

@toddsouthenbentley toddsouthenbentley Jul 22, 2024

Choose a reason for hiding this comment

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

I would suggest calling useEffect instead of useMemo since the latter is meant for memoizing calculated data (i.e. storing the return value of useMemo).

I think having Field update their code to call useActiveIModelConnection makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

If SYNCHRO agrees with us removing useIModel, this discussion is moot, but based on my understanding of how useEffect and useMemo work, useEffect will cause an extra render at startup and every time the value changes. Having said that, I am well aware that this is a very non-standard usage of useMemo, so maybe the extra render is not something to worry about.

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