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

KOGITO-9831: SWF Editor - final migration to j2cl #1990

Merged
merged 30 commits into from
Nov 8, 2023

Conversation

treblereel
Copy link
Contributor

@yesamer
Copy link
Contributor

yesamer commented Oct 3, 2023

Hi @treblereel, a side note: I noticed you introduced some classes with Copyright © 2023 Treblereel header. I think that is not ideal according to the Apache rules. Is that header intended? If yes, can you please update the NOTICE file, using the same pattern already in place? Thank you!

//cc @ederign @tiagobento

@treblereel treblereel temporarily deployed to ci October 3, 2023 11:14 — with GitHub Actions Inactive
@treblereel treblereel temporarily deployed to ci October 3, 2023 11:14 — with GitHub Actions Inactive
@treblereel
Copy link
Contributor Author

Hi @treblereel, a side note: I noticed you introduced some classes with Copyright © 2023 Treblereel header. I think that is not ideal according to the Apache rules. Is that header intended? If yes, can you please update the NOTICE file, using the same pattern already in place? Thank you!

//cc @ederign @tiagobento

Thanks for the tip, I'll double-check it.

@treblereel treblereel temporarily deployed to ci October 4, 2023 16:40 — with GitHub Actions Inactive
@treblereel treblereel temporarily deployed to ci October 4, 2023 16:40 — with GitHub Actions Inactive
@treblereel treblereel temporarily deployed to ci October 5, 2023 03:20 — with GitHub Actions Inactive
@treblereel treblereel temporarily deployed to ci October 5, 2023 03:20 — with GitHub Actions Inactive
@treblereel treblereel temporarily deployed to ci October 5, 2023 03:21 — with GitHub Actions Inactive
@LuboTerifaj LuboTerifaj self-requested a review October 5, 2023 12:50
@handreyrc handreyrc self-requested a review October 5, 2023 18:06
Copy link
Contributor

@handreyrc handreyrc left a comment

Choose a reason for hiding this comment

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

Hey @treblereel;

I noticed we are reintroducing the patternfly files removed in the KOGITO-9728.
We have to keep the same approach. We cannot have those files as part of our source.

Could you check it please?

Thanks

@treblereel
Copy link
Contributor Author

Hey @treblereel;

I noticed we are reintroducing the patternfly files removed in the KOGITO-9728. We have to keep the same approach. We cannot have those files as part of our source.

Could you check it please?

Thanks

Thank you, it ll be fixed

@treblereel treblereel temporarily deployed to ci October 12, 2023 07:18 — with GitHub Actions Inactive
@treblereel treblereel temporarily deployed to ci October 12, 2023 07:19 — with GitHub Actions Inactive
@treblereel treblereel temporarily deployed to ci October 12, 2023 07:19 — with GitHub Actions Inactive
@treblereel treblereel force-pushed the FINAL_J2CL_MIGRATION branch from 8dbd238 to da1443f Compare October 27, 2023 21:11
@treblereel treblereel temporarily deployed to ci October 27, 2023 21:11 — with GitHub Actions Inactive
@treblereel treblereel temporarily deployed to ci October 27, 2023 21:11 — with GitHub Actions Inactive
@treblereel treblereel temporarily deployed to ci October 27, 2023 21:11 — with GitHub Actions Inactive
@treblereel
Copy link
Contributor Author

Hello @treblereel ,

Code

There is still gwt used in some modules. Shouldn't be all its usages removed in this PR?

There is commented out code in the following Test classes. Could you please ensure there won't stay any leftovers?

  • EditorReadOnlyProviderTest, AbstractCanvasShortcutsControlImplTest, ElementProxyTest, CreateNodeToolboxActionTest, FlowActionsToolboxFactoryTest, GeneralCreateNodeActionTest, GroupActionsToolboxFactoryTest, MorphActionsToolboxFactoryTest, MorphNodeToolboxActionTest, SaveDiagramSessionCommandTest, StunnerEditorTest

There is a commented out code in the following classes. Is there any reason why we need to keep it there?

  • AbstractModelBeanValidator, ModelBeanViolationImpl, ValidationSuccessNotification, WorkbenchEntryPoint

There is a TODO in org.uberfire.client.promise.Promises class. Please ensure you don't forget on that.

Behaviour

"null" instad of "undefined" is shown in a tooltip when hovering over:

Additional notes

Could you please resolve conflicts in your PR?

All integration tests passed. I didn't find any other issues.

Thank you!

Thank you for your review!

@handreyrc handreyrc self-requested a review October 27, 2023 22:07
Copy link
Contributor

@handreyrc handreyrc left a comment

Choose a reason for hiding this comment

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

@treblereel,

Everything looks good to me. I didn't find any regressions.

Thanks!

@LuboTerifaj
Copy link
Contributor

Running integration tests, I will let you know once they are finished.

Copy link
Contributor

@LuboTerifaj LuboTerifaj left a comment

Choose a reason for hiding this comment

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

Hello @treblereel ,

Thanks for the updates.

I reported the issue that is not fixed here:

All tests passed and I didn't find any more issues.

@caponetto
Copy link
Contributor

Hi @treblereel, I found a minor issue while comparing the changes in this PR with the current release of Serverless Logic Web Tools. Not a blocker but is it a known issue?

You can see in red that an icon is missing when running it from this branch. The image below is the current release.

image

@treblereel
Copy link
Contributor Author

treblereel commented Nov 6, 2023

Hi @treblereel, I found a minor issue while comparing the changes in this PR with the current release of Serverless Logic Web Tools. Not a blocker but is it a known issue?

You can see in red that an icon is missing when running it from this branch. The image below is the current release.

image

@caponetto
nope, it's not, lets file the issue, looks like font-awesome font is missed.

And looks like it's specific to your case, coz in web/vscode it works as expected.

@ederign ederign merged commit 02cab1e into apache:main Nov 8, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants