feat: jira plugin page #4627
feat: jira plugin page #4627
Conversation
sjaanus
commented
Sep 7, 2023
sjaanus
commented
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -337,6 +338,14 @@ export const routes: IRoute[] = [ | |||
| type: 'protected', | |||
| menu: {}, | |||
| }, | |||
| { | |||
| path: '/integrations/view/:providerId', | |||
There was a problem hiding this comment.
Currently this serves only static page on Jira, but if more pages come, there might be possibility to make it generic.
There was a problem hiding this comment.
No blockers, happy for you to merge this if it gives you more momentum 😄 However, I do have a few points that I think should be addressed either before merging or in a follow-up. Musings on whether variables will be present are completely optional and just thoughts I have. But the points on valid HTML structure and accessibility I think should be taken into account and fixed.
Happy to take any questions or hear thoughts
| return ( | ||
| <StyledContainer> | ||
| <StyledTitle>{title}</StyledTitle> | ||
| <Typography>{description}</Typography> | ||
| <StyledImg src={formatAssetPath(src)} /> | ||
| </StyledContainer> | ||
| ); |
There was a problem hiding this comment.
What HTML elements is this rendered as (and what kind of a thing is it)? From the context, it sounds as if it might be an image with a caption? In that case, should we use the figure and caption elements? And should the title be an h3 or something similar (depending on its position in the page)? If you wanna be very nitty gritty, the StyledContainer could probably be an article instead of a div, too, but that's not very important.
Also, the images don't have any captions or alt text, which is an accessibility issue. Based on the usage in the new code, I think the typography should be put in a figcaption component, linked to the images figure component.
There was a problem hiding this comment.
Did you push the changes? For some reason, I don't think I'm seeing them? 🤔
| gap: theme.spacing(2), | ||
| flexDirection: 'column', | ||
| })); | ||
| export const StyledTitle = styled(Typography)({ |
There was a problem hiding this comment.
Based on the usage (and my understanding from the screenies you posted), it feels like this should be an h2 element if I'm reading it right? But that's a minor thing, and I'll leave it up to you, whether or not to handle that.
| export const StyledTitle = styled(Typography)({ | |
| export const StyledTitle = styled('h2')({ |
