-
Notifications
You must be signed in to change notification settings - Fork 65
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
Execution Environment add or edit #1054
Conversation
84e01b7
to
8094836
Compare
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.
Milan and I discussed these changes off line. This is a great start. Let make some changes to help all AAP UI projects unify around some conventions.
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
a9c8b78
to
7c0e968
Compare
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.
@MilanPospisil I got the latest version locally, and it seems to be working as expected.
Good job.
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.
Just update that 1 string and I think we will be good to go.
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
frontend/hub/execution-environments/ExecutionEnvironmentForm.tsx
Outdated
Show resolved
Hide resolved
The problem where clicking on the "Show less" button in the LabelGroup causes a reload is unrelated to this PR, that's a patternfly bug:
The problem is that pattenrfly Label renders a raw So, the solution is to fix patternfly, either of these works: diff --git a/packages/react-core/src/components/Label/Label.tsx b/packages/react-core/src/components/Label/Label.tsx
index 0bab0335a..101b5c894 100644
--- a/packages/react-core/src/components/Label/Label.tsx
+++ b/packages/react-core/src/components/Label/Label.tsx
@@ -180,7 +180,8 @@ export const Label: React.FunctionComponent<LabelProps> = ({
}
};
- const LabelComponent = (isOverflowLabel ? 'button' : 'span') as any;
+ const safeButton = ({ children, ...props }) => <button type="button" {...props}>{children}</button>;
+ const LabelComponent = (isOverflowLabel ? safeButton : 'span') as any;
const button = closeBtn ? (
closeBtn OR diff --git a/packages/react-core/src/components/LabelGroup/LabelGroup.tsx b/packages/react-core/src/components/LabelGroup/LabelGroup.tsx
index 82e8d0376..ba449b3d9 100644
--- a/packages/react-core/src/components/LabelGroup/LabelGroup.tsx
+++ b/packages/react-core/src/components/LabelGroup/LabelGroup.tsx
@@ -104,7 +104,9 @@ export class LabelGroup extends React.Component<LabelGroupProps, LabelGroupState
});
}
- toggleCollapse = () => {
+ toggleCollapse = (e) => {
+ e.preventDefault();
+
this.setState(prevState => ({
isOpen: !prevState.isOpen,
isTooltipVisible: Boolean( (not sure how that interacts with the pf5 switch) |
7c0e968
to
c40a722
Compare
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.
@MilanPospisil informed that will the last framework changes in a follow up PR.
5c074eb
to
8c92e4a
Compare
0c160d0
to
37c4390
Compare
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.
Looks good. All of my concerns have been addressed. Well done!
No description provided.