pierrejeambrun commented on code in PR #54163:
URL: https://github.com/apache/airflow/pull/54163#discussion_r2269732019
##########
airflow-core/src/airflow/ui/src/components/EditableMarkdownButton.tsx:
##########
@@ -53,18 +53,33 @@ const EditableMarkdownButton = ({
return (
<Box>
- <ActionButton
- actionName={placeholder}
- icon={icon}
- onClick={() => {
- if (!isOpen) {
- onOpen();
- }
- setIsOpen(true);
- }}
- text={text}
- withText={withText}
- />
+ <Box display="inline-block" position="relative">
+ <ActionButton
+ actionName={placeholder}
+ icon={icon}
+ onClick={() => {
+ if (!isOpen) {
+ onOpen();
+ }
+ setIsOpen(true);
+ }}
+ text={text}
+ variant="outline"
+ withText={withText}
+ />
+ {Boolean(mdContent?.trim()) && (
+ <Box
+ bg="blue.500"
+ borderRadius="full"
+ height="2.5"
+ position="absolute"
+ right="-0.5"
+ top="-0.5"
+ width="2.5"
Review Comment:
```suggestion
height={2.5}
position="absolute"
right={-0.5}
top={-0.5}
width={2.5}
```
##########
airflow-core/src/airflow/ui/src/pages/Run/Header.tsx:
##########
@@ -43,6 +43,10 @@ export const Header = ({
const { t: translate } = useTranslation();
const [note, setNote] = useState<string | null>(dagRun.note);
+ useEffect(() => {
+ setNote(dagRun.note);
+ }, [dagRun.note]);
+
Review Comment:
Some useEffects are fine, but most of the time they are not necessary, and
tend to trigger extra rendering and slow down the application.
So we tend to try to limit those, unless they are really needed.
You can take a look at `text={Boolean(dagRun.note) ? translate("note.label")
: translate("note.add")}` this propery of the `EditableMarkdownButton` which is
doing something really similar to what you are trying to achieve. (and is
working for the change of label between runs)
I think we should refactor this into maybe `hasContent`, this would allow to
know if we should render the 'dot' and what text should be chosen in the
`EditableMarkdownButton`.
Removing the need for those two useEffects.
##########
airflow-core/src/airflow/ui/src/components/EditableMarkdownButton.tsx:
##########
@@ -53,18 +53,33 @@ const EditableMarkdownButton = ({
return (
<Box>
- <ActionButton
- actionName={placeholder}
- icon={icon}
- onClick={() => {
- if (!isOpen) {
- onOpen();
- }
- setIsOpen(true);
- }}
- text={text}
- withText={withText}
- />
+ <Box display="inline-block" position="relative">
+ <ActionButton
+ actionName={placeholder}
+ icon={icon}
+ onClick={() => {
+ if (!isOpen) {
+ onOpen();
+ }
+ setIsOpen(true);
+ }}
+ text={text}
+ variant="outline"
+ withText={withText}
+ />
+ {Boolean(mdContent?.trim()) && (
+ <Box
+ bg="blue.500"
+ borderRadius="full"
+ height="2.5"
+ position="absolute"
+ right="-0.5"
+ top="-0.5"
+ width="2.5"
+ zIndex={1}
Review Comment:
I don't think that's necessary
```suggestion
```
##########
airflow-core/src/airflow/ui/src/components/EditableMarkdownButton.tsx:
##########
@@ -53,18 +53,33 @@ const EditableMarkdownButton = ({
return (
<Box>
- <ActionButton
- actionName={placeholder}
- icon={icon}
- onClick={() => {
- if (!isOpen) {
- onOpen();
- }
- setIsOpen(true);
- }}
- text={text}
- withText={withText}
- />
+ <Box display="inline-block" position="relative">
+ <ActionButton
+ actionName={placeholder}
+ icon={icon}
+ onClick={() => {
+ if (!isOpen) {
+ onOpen();
+ }
+ setIsOpen(true);
+ }}
+ text={text}
+ variant="outline"
+ withText={withText}
+ />
+ {Boolean(mdContent?.trim()) && (
+ <Box
+ bg="blue.500"
+ borderRadius="full"
+ height="2.5"
+ position="absolute"
+ right="-0.5"
+ top="-0.5"
+ width="2.5"
Review Comment:
nit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
