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.

Sign up for GitHub

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

Jump to bottom

feat(chrome-ext): support states #1787

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(chrome-ext): support states #1787

wants to merge 1 commit into from

Conversation

matthieu-crouzet
Copy link
Contributor

@matthieu-crouzet matthieu-crouzet commented May 15, 2024 •
edited

Proposed change

image

  • Add current language in state (if has been changed?)
  • Add the schema in state json exported file
  • Validate the json imported file with the schema
  • Add reset button when configuration is changed
  • Choose between the save of only the override or all the current value
    • Split this choice by category (theming / localization / configuration / language)
  • Support the capability to close the chrome extension and reopen without loosing unsaved changes
    • Or find a way to block the closing of the devtools if unsaved changes and ask if they want to save it or not
  • Add capability to let the app select the state to activate on bootstrap (query param?)
  • Move the save button in the select of the states and remove the warning border
  • Add new import state methods
    • By URL
    • By textarea

Related issues

@matthieu-crouzet matthieu-crouzet requested a review from a team as a code owner May 15, 2024 08:24
@nx-cloud Nx Cloud
Copy link

nx-cloud bot commented May 15, 2024 •
edited

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 6bdd008. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 10 targets

Sent with 💌 from NxCloud.

@matthieu-crouzet matthieu-crouzet force-pushed the feat/chrome-state branch 2 times, most recently from 774047e to afd4fd2 Compare May 15, 2024 11:43
sdo-1A
sdo-1A reviewed May 17, 2024
View reviewed changes
apps/chrome-devtools/src/app-devtools/state-panel/state-panel.component.ts
@Component({
selector: 'o3r-state-panel',
templateUrl: './state-panel.template.html',
styles: `
Copy link
Contributor

Choose a reason for hiding this comment

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

should the styles be in an scss file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both solutions are okay

@sdo-1A
Copy link
Contributor

sdo-1A commented May 17, 2024

add the state feature to the apps/chrome-devtools/README.md

@matthieu-crouzet matthieu-crouzet force-pushed the feat/chrome-state branch 2 times, most recently from e7d8590 to d9cfc16 Compare May 17, 2024 14:21
kpanot
kpanot reviewed May 20, 2024
View reviewed changes
apps/chrome-devtools/schemas/state.schema.json
"name"
],
"properties": {
"color": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing description to the properties

apps/chrome-devtools/src/app-devtools/app.component.html
</ng-template>
</li>
</ul>
<form [formGroup]="form" class="px-4 pt-{{hasLocalChanges() ? '2' : '3'}} ng-pristine ng-valid ng-touched border-bottom ms-auto">
Copy link
Contributor

Choose a reason for hiding this comment

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

let's be CSS optimizer ready and not cut css class.

Suggested change
<form [formGroup]="form" class="px-4 pt-{{hasLocalChanges() ? '2' : '3'}} ng-pristine ng-valid ng-touched border-bottom ms-auto">
<form [formGroup]="form" class="px-4 {{hasLocalChanges() ? 'pt-2' : 'pt-3'}} ng-pristine ng-valid ng-touched border-bottom ms-auto">

apps/chrome-devtools/src/manifest.json
@@ -7,6 +7,7 @@
"permissions": [
"activeTab",
"scripting",
"storage",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not forget to add a justification to this permission to the Chrome Developer Portal when publishing this new version (Otherwise the publication maybe rejected)

cpaulve-1A
cpaulve-1A reviewed May 27, 2024
View reviewed changes
apps/chrome-devtools/src/app-devtools/app.component.ts
this.debugPanelService.update(msg);
}
});
const activateStateNameFormValueChanges = toSignal(this.form.controls.activeStateName.valueChanges, { initialValue: this.activeStateName() });
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be nice to establish some recommendation and consistency over the code on how we handle the link between a form valueChange and a signal

cpaulve-1A
cpaulve-1A reviewed May 27, 2024
View reviewed changes
apps/chrome-devtools/src/app-devtools/state-panel/state-panel.component.ts
this.stateService.setActiveState(stateName);
}

public updateState(stateName: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing tsdoc for this file

cpaulve-1A
cpaulve-1A reviewed May 27, 2024
View reviewed changes
apps/chrome-devtools/src/app-devtools/state-panel/state-panel.component.ts
if (!el) {
return;
}
el.nativeElement.innerHTML = (e as Error).message;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not handle the importStateText as another signal?

cpaulve-1A
cpaulve-1A reviewed May 27, 2024
View reviewed changes
apps/chrome-devtools/src/extension/background.ts
@@ -31,6 +37,64 @@ const isInjectionContentMessage = (content: any): content is InjectContentMessag
return content?.dataType === 'inject';
};


const isApplicationInformationMessage = (data?: OtterMessageContent): data is ApplicationInformationContentMessage => data?.dataType === 'applicationInformation';
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing tsdoc in this file

@cpaulve-1A
Copy link
Contributor

We need to provide a documentation to help a user with this feature.
It should answer the following questions:

  • how to whitelist a domain
  • how to activate a state - do they need to do it manually on the extension via the debugger tool or is it automatic?
  • do we support the same state in two different tabs ? (What happens in this case?)

kpanot
kpanot reviewed Jun 5, 2024
View reviewed changes
apps/chrome-devtools/src/extension/background.ts
import type { ExtensionMessage } from './interface';
import type { InjectContentMessage, OtterMessage, OtterMessageContent, otterMessageType } from '@o3r/core';
import type { ApplicationInformationContentMessage } from '@o3r/application';
import { type scriptToInject as ScriptToInject } from '../services/connection.service';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { type scriptToInject as ScriptToInject } from '../services/connection.service';
import type { scriptToInject as ScriptToInject } from '../services/connection.service';

do not change anything but accentuate the point that only import type is allow in this file

apps/chrome-devtools/src/services/localization.service.ts
),
{ initialValue: false }
);
private readonly lang = signal<string | undefined>(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be declared before public field.
I was thinking the linter would check it :S

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kpanot kpanot kpanot left review comments

@cpaulve-1A cpaulve-1A cpaulve-1A left review comments

@sdo-1A sdo-1A sdo-1A approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request project:@o3r/application project:@o3r/chrome-devtools project:@o3r/localization project:@o3r/showcase project:@o3r/styling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@matthieu-crouzet @sdo-1A @cpaulve-1A @kpanot