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

Remove old todos #3851

Merged
merged 12 commits into from
Aug 12, 2024
Merged

Remove old todos #3851

merged 12 commits into from
Aug 12, 2024

Conversation

ndricimrr
Copy link
Contributor

@ndricimrr ndricimrr commented Aug 8, 2024

Description

Changes proposed in this pull request:

  • Remove old todos that are out of context or outdated

part of #3532

@ndricimrr ndricimrr added the refactoring Something needs to be refactored in a separate task label Aug 8, 2024
@ndricimrr ndricimrr assigned ndricimrr and unassigned ndricimrr Aug 9, 2024
@@ -127,7 +127,7 @@
compound: compoundConfig,
viewUrl: viewurl,
webcomponent: GenericHelperFunctions.checkWebcomponentValue(webcomponent) || true
}; // TODO: fill with sth
Copy link
Contributor Author

Choose a reason for hiding this comment

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

missing context

},
'*'
);
break;
case LuigiInternalMessageID.NAVIGATION_REQUEST:
this.dispatch(Events.NAVIGATION_REQUEST, targetCnt, event.data.params);
break;
// TODO 1: handle alerts with ids on next iteration
Copy link
Contributor Author

Choose a reason for hiding this comment

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

already handled

@@ -164,7 +163,6 @@ export class ContainerService {
case LuigiInternalMessageID.GET_CURRENT_ROUTE_REQUEST:
this.dispatch(Events.GET_CURRENT_ROUTE_REQUEST, targetCnt, event);
break;
// TODO: discuss if actually needed as the only scenario is when microfrontend initially starts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed as it needs to fit to existing LuigiClient

@@ -746,7 +746,6 @@
msg: 'luigi.ux.alert.hide',
id,
dismissKey
//TODO: update docu for this param
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -364,7 +364,7 @@ export class WebComponentService {

if (wc.__postProcess) {
const url =
new URL(document.baseURI).origin === new URL(viewUrl, document.baseURI).origin // TODO: check if needed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

out of context

@ndricimrr ndricimrr changed the title Resolve old todos 1 Remove old todos Aug 9, 2024
Copy link
Contributor

@walmazacn walmazacn left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@walmazacn walmazacn self-assigned this Aug 9, 2024
@ndricimrr ndricimrr merged commit 64fe20e into main Aug 12, 2024
12 checks passed
@ndricimrr ndricimrr deleted the resolve-todos-1 branch August 12, 2024 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Something needs to be refactored in a separate task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants