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

Update accudraw core 4 11 #1169

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

mathieu-fournier
Copy link
Contributor

Accudraw updates for ITwin-Core 4.11

ITwin core 4.11 is not released yet, thus the Draft PR.

Apply updates required by these PRs in Core :
itwinjs-core/pull/7488
itwinjs-core/pull/7492

Input field now updates to the formatted value when losing focus.
Using the updated API.

@@ -156,14 +161,10 @@ export function AccuDrawFieldContainer(props: AccuDrawFieldContainerProps) {

React.useEffect(() => {
// Set the focus to the first input field when the component is mounted and when the compass mode changes.
const itemToFocus =
mode === CompassMode.Polar ? ItemField.DIST_Item : ItemField.X_Item;
const itemToFocus = IModelApp.accuDraw.defaultFocusItem();
Copy link
Collaborator

Choose a reason for hiding this comment

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

IModelApp.accuDraw.defaultFocusItem does this exist in @itwin/[email protected]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it'll be available in 4.11

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to support ^4.0.0 as indicated in package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bbastings
Any idea to make this fix, retro compatible to ^4.0.0 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just make local copies for now, or keep the old code. These changes aren't enough to fix AccuDraw anyway, I'm still working on it and nothing is getting merged into anything before 4.11.

public defaultFocusItem(): ItemField { return (CompassMode.Polar === this.compassMode ? ItemField.DIST_Item : this.newFocus); }

Will complain about missing override once itwin core packages are updated so you'll know it can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GerardasB
Can we just bump the peer to 4.11 when released ?

Copy link
Collaborator

@GerardasB GerardasB Jan 16, 2025

Choose a reason for hiding this comment

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

No, since that requires a new major version. Before calling a potentially nonexistent API simply check if it is defined. Alternatively - maybe https://www.itwinjs.org/reference/core-frontend/imodelapp/itwinjs_core_version/ can be used.
In either case - seems like there needs to be some branching to support both pre and after 4.11

async function acceptInputValue(itemField: ItemField) {
await AccuDrawShortcuts.itemFieldAcceptInput(
itemField,
IModelApp.accuDraw.getFormattedValueByIndex(itemField)
Copy link
Collaborator

Choose a reason for hiding this comment

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

IModelApp.accuDraw.getFormattedValueByIndex does this exist in @itwin/[email protected]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it'll be available in 4.11

Copy link
Contributor

Choose a reason for hiding this comment

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

This requires the changes to add the parsers to core AccuDraw, so yeah...f we can't upgrade to 4.11 may as well just revert these changes.

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