-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat: Info Center for different type of messages #2909
base: main
Are you sure you want to change the base?
Conversation
…/2511/info_center
…/2511/info_center
…/2511/info_center
…/2511/info_center
…/2511/info_center
…/2511/info_center
🦋 Changeset detectedLatest commit: 4c9a8c3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/control-property-editor/src/panels/info-center/InfoCenter.scss
Outdated
Show resolved
Hide resolved
packages/control-property-editor/src/panels/info-center/InfoCenter.scss
Outdated
Show resolved
Hide resolved
packages/control-property-editor/src/panels/info-center/InfoCenter.tsx
Outdated
Show resolved
Hide resolved
packages/control-property-editor/src/panels/info-center/InfoCenter.tsx
Outdated
Show resolved
Hide resolved
packages/control-property-editor/src/panels/info-center/InfoCenter.tsx
Outdated
Show resolved
Hide resolved
packages/control-property-editor/src/panels/info-center/InfoCenter.tsx
Outdated
Show resolved
Hide resolved
expandable?: boolean; | ||
expanded?: boolean; | ||
read?: boolean; | ||
modal?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only need to define the data types that are required for communication between the client and the editor. All the UI state should be defined in control-property-editor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved
export const clearInfoCenterMessage = createExternalAction<number>('clear-info-center-message'); | ||
export const clearAllInfoCenterMessages = createExternalAction<void>('clear-all-info-center-message'); | ||
export const externalFileChange = createExternalAction<string>('external-file-change'); | ||
export const toggleExpandMessage = createExternalAction<number>('toggle-expand-message'); | ||
export const readMessage = createExternalAction<number>('read-message'); | ||
export const expandableMessage = createExternalAction<number>('expandable-message'); | ||
export const toggleModalMessage = createExternalAction<number>('toggle-modal-message'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these look like UI actions, so they should also be only defined in the control-property-editor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
shouldHideIframe: false | ||
showInfoCenterMessage({ | ||
message: { | ||
title: 'Reuse components detected', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Title should also use texts from message bundle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done for all texts
@@ -19,6 +19,7 @@ $splitter-focus-border: 1px solid var(--vscode-focusBorder); | |||
background-color: var(--vscode-panelSection-border); | |||
position: absolute; | |||
outline: none; | |||
z-index: 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this needed? It seems that info center panel overlaps a bit the splitter, but that seems like an issue with info center panels styling not the splitter, because other components are not affected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted and fixed in the info center styles
</Text> | ||
{isExpandable && ( | ||
<Text className="more-less" onClick={() => dispatch(toggleExpandMessage(index))}> | ||
{isExpanded ? 'Less' : 'More'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the UI texts should use i18n.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
<UIDialog | ||
hidden={!isOpenedModal} | ||
dialogContentProps={{ | ||
title: 'Error Details' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checked ui-components
:
- code changes are easy to understand - I did not notice issue
- tests are added for additional message types
} | ||
|
||
.message-description { | ||
display: -webkit-box; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a short comment why browser prefixes are needed here would be good as we generally try to avoid them and that this is required for the line-clamp
is not obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment description
background-color: var(--vscode-actionBar-toggledBackground) !important; | ||
} | ||
|
||
.message-title { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each component should have a dedicated style file. Message related styles should be in InfoMessageItem.scss
file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Divided into separate files
@@ -35,7 +35,7 @@ ADP_ADD_FRAGMENT_NOTIFICATION = Note: The "{0}.fragment.xml" fragment will be cr | |||
ADP_ADD_FRAGMENT_WITH_TEMPLATE_NOTIFICATION = Note: The "{0}.fragment.xml" fragment will be created once you save the changes. | |||
ADP_ADD_TWO_FRAGMENTS_WITH_TEMPLATE_NOTIFICATION = Note: The "{0}.fragment.xml" and "{1}.fragment.xml" fragments will be created once you save the changes. | |||
ADP_SYNC_VIEWS_MESSAGE = Synchronous views are detected for this application. Controller extensions are not supported for such views and will be disabled. | |||
ADP_REUSE_COMPONENTS_MESSAGE = Reuse components are detected for some views in this application. Controller extensions, adding fragments and manifest changes are not supported for such views and will be disabled. | |||
ADP_REUSE_COMPONENTS_MESSAGE = Reuse components detected | Reuse components are detected for some views in this application. Controller extensions, adding fragments and manifest changes are not supported for such views and will be disabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is rather confusing, looking at this text it is not clear that it is actually used for two different things.
Please add a new entry for the title instead of combining two and then splitting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new entry for the title was added
shouldHideIframe: false | ||
showInfoCenterMessage({ | ||
type: MessageBarType.warning, | ||
title: title.trim(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to perform any operations on the texts. Changes should be done in .properties files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
@@ -35,6 +40,7 @@ export class OutlineService extends EventTarget { | |||
const resourceBundle = await getTextBundle(); | |||
const key = 'ADP_REUSE_COMPONENTS_MESSAGE'; | |||
const message = resourceBundle.getText(key) ?? key; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably can be converted into one liner passing the key without variable, getText
should already return the value of key
if the text is not found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
packages/control-property-editor/src/panels/info-center/InfoMessageItem.tsx
Show resolved
Hide resolved
packages/control-property-editor/src/panels/info-center/InfoMessageItem.tsx
Outdated
Show resolved
Hide resolved
…x-tools into feat/2511/info_center
packages/control-property-editor/src/panels/info-center/InfoMessageItem.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Nikita B. <[email protected]>
|
Feat for #2511