-
Notifications
You must be signed in to change notification settings - Fork 149
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
Fix linebreak follow common mark convention #3626
Conversation
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.
Nice catch!
@@ -184,6 +184,7 @@ export const ApplicationDetail: FC<ApplicationDetailProps> = memo( | |||
|
|||
const piped = useAppSelector(selectPipedById(app?.pipedId)); | |||
const isSyncing = useIsSyncingApplication(app?.id); | |||
const description = app?.description.replace(/\\\n/g, " \n"); |
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.
Nice solution.
@@ -319,7 +320,7 @@ export const ApplicationDetail: FC<ApplicationDetailProps> = memo( | |||
display="flex" | |||
> | |||
<ReactMarkdown linkTarget="_blank" className={classes.markdown}> | |||
{app.description} | |||
{description || "No description."} |
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 do we need this?
I feel that the current empty string is better since it can hide the unneeded description block.
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.
I get your point 👍 But to me, showing that there will be something there in case of the configuration file contains a description field would be better for the UI, and users who have not yet used that feature will be aware of that
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.
The design is still not good enough for that empty case and we're having Play environment to show up that feature.
So I think that it should be hidden until we have a better design. 🤔
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 better UI is a long time issue of pipecd development 😄 so well, if you guys really think that the current description is ugly, then it's okay to me to remove that if it's empty
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, updated by b36e46c
web/src/components/application-detail-page/application-detail/index.tsx
Outdated
Show resolved
Hide resolved
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.
Thank you.
What this PR does / why we need it:
In this PR, I added
No description.
text in case the application configuration does not contain any description value.\
(follow commonMark convention) to markdown best practice hard line break (with two or more spaces at the end of line)Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: