-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: Allow markdown in workflow title and description. Fixes #10126 #10553
feat: Allow markdown in workflow title and description. Fixes #10126 #10553
Conversation
753c778
to
5cedf49
Compare
Seems CI is having a rough time the last few runs.
Don't see how I can re-run without pushing another commit. If I'm able to re-run I'm not seeing it. |
ui/src/app/workflows/components/workflows-row/workflows-row-name.tsx
Outdated
Show resolved
Hide resolved
fc999fb
to
9e5f065
Compare
Unfortunately, there are intermittent failures. You can push empty commits until it all passes. |
@juliev0 yeah, I've done that before. I think the last few errors were legit re: yarn.lock. Fixed them I think 🤞 |
@juliev0 @alexec @terrytangyuan CI finally passed. Would love feedback/approval 😄 pretty please. |
498ce6f
to
adafc2d
Compare
@alexec @terrytangyuan either of you available for a review here? If not, who might be a person to ping? Any UI folks I should query? Thank you for any assistance. Cheers. |
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 work! Thanks for the example. Could you please add doc under "User Guide -> UI features" titled something like "Workflow descriptions" so we have this feature documented in addition to your example?
We unfortunately don't have workflow descriptions documented clearly so it's an under-used feature in my opinion.
I imagine we'll be able to get this merged to master
in a March once an approver signs off.
b95bc24
to
b39161e
Compare
@caelan-io docs page added. Great suggestion. |
3636854
to
2e85240
Compare
Build error is |
81d5192
to
e0075a8
Compare
@alexec checks pass and UI running for me locally |
@alexec could the This might be the culprit with the I need to retest locally with node 16 now. But.....the CI tests pass. Either good or concerning. TBD. |
My PR requires the upgrade to I'm also having to add This was working with node 19 but no longer. Now I have to hunt what else changed. I see the |
6c06fcf
to
02af38b
Compare
02af38b
to
8162a67
Compare
Current issue is the entire workflow row is a link. Have to do some CSS to make the markdown links open their endpoints instead of the workflow detail view in the argo-workflows dashboard. |
f423a28
to
15f7251
Compare
Looking into the test failures. |
17e3e70
to
a9efa36
Compare
Fixes: argoproj#10126 - [x] new react component for handling workflow row name - [x] new css file to manage row name column - [x] new example file for title and description with markdown - [x] include `remark-gfm` for GitHub Friendly Markdown (aka urls automatically translate to links) - [x] Add User Guid -> UI Features -> Title and Description docs page - [x] make first column of workflow row not be wrapped in row link so markdown links can be clicked - [x] add overlay so users can still click to open workflow - [x] change remark-gfm to 3.0.0 based on remarkjs/react-markdown#771 (comment) Signed-off-by: jmeridth <[email protected]> Co-authored-by: Steven Johnson <[email protected]>
a9efa36
to
416139f
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.
LGTM
…j#10126 (argoproj#10553) Signed-off-by: jmeridth <[email protected]> Co-authored-by: Steven Johnson <[email protected]>
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.
So this ended up getting merged without a UI reviewer on it. There are some issues with the feature as written though, some of which I immediately noticed. There's a typo, a bug in the columns, and some issues with the docs too
For posterity, this was also a topic of discussion in the Jan 2nd Contributor Meeting (which I couldn't attend as it's the biggest holiday for my family)
"react-monaco-editor": "^0.50.1", | ||
"react-router-dom": "^4.2.2", | ||
"remark-gfm": "^3.0.0", |
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.
Do we really need GFM? The features it adds is fairly small, but it is an entire new dep. Most of those features would also not be used, e.g. tables and task lists don't really make sense in this context.
In general, a smaller dep that supports less features would make more sense, as full-featured Markdown in an annotation has some issues and being displayed inside of a list also is a specific context
Given that I spent a solid amount of time improving the UI load time with code-splitting for #12059, we really should be even more careful than we already should be when introducing new, large deps, especially in the UI
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.
On Slack we discussed potentially putting the whole markdown feature behind a feature flag as otherwise the deps load unconditionally for all users, even those who don't use this feature
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.
So we can do an initial code-split and only load the deps if the title
and description
annotations exist, but it may be difficult to discern if those have markdown specifically without a dep, which is where a feature flag might be good. I still would prefer not to gate it behind a feature flag, but I also don't want the deps loaded for users who don't use the feature.
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.
Code-split this in #12580 based on if the annotations exist, but as written above, can't quite determine if those have markdown without a dep, so there is potentially further action to do here. Maybe there's a clever one-line regex to do it though and then we wouldn't need a dep to detect markdown
Also we did discuss the XSS concern offline, JM chose |
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.
left some notes on the hacks
@@ -41,16 +42,16 @@ export function WorkflowsRow(props: WorkflowsRowProps) { | |||
/> | |||
<PhaseIcon value={wf.status.phase} /> | |||
</div> | |||
<div className='columns small-2'> | |||
<a className='overlay' href={workflowLink}></a> |
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 indeed quite hacky as you mentioned in the PR description. I also can't seem to expand the row right now, clicking the "Show" button takes me to the Workflow Details page.
instead of using an a
tag, you can use a different element and capture the event and then use navigation.goto
to change the route (which is what the Link
component does under-the-hood). clicks elsewhere should capture the event first and prevent it from bubbling up to this level.
but tbh, I don't like that the whole thing is a link in the first place, that's very confusing UX-wise as there are different behaviors based on where/when you click. and there's a bunch of text that one may want to copy+paste or highlight, which is basically impossible right now.
so I actually think we should change up the behavior anyway, but I'm not yet quite sure where to put the link to the details page then. maybe we just make the name link to to the Workflow Details and remove the link from everywhere else.
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.
So I removed the whole row being a link in #12580 and made the name only a link.
But then the nested linking issue re-appeared, so I used JS as I mentioned above to prevent bubble up, which is a lot less hacky and less problematic then the overlay CSS (although it still has some semantic issues, as I mention in #12580, but they're technically an edge of an edge case)
Fixes: #10126
remark-gfm
for GitHub Friendly Markdown (aka urls automatically translate to links)Examples by row:
remark-gfm
to auto turn urls into linksNotes:
NOTES
We have 2 choices as I see it regarding how to handle the possible nested links from the markdown generated HTML
Link
react componentDownside for both choices - full page refresh, not section refresh like
Link
componentWhy move the name column out from under the
Link
component?When the Name column was under the
Link
component, clicking of markdown generated links always still redirected users to the workflow due to theLink
react component negating default behavior of the link.When we pressed cmd/ctrl+link and opened the markdown link in a new tab, however, it worked. This doesn't help us because we have no control of the anchors (links) that are generated from the markdown.
Please do not open a pull request until you have checked ALL of these:
make pre-commit -B
to fix codegen and lint problems.If changes were requested, and you've made them, dismiss the review to get it reviewed again.