Edit Page Enhancements #1319
Edit Page Enhancements #1319
Conversation
There was a problem hiding this comment.
This is really awesome! I left some suggestions, but this is shaping up to be great, and my experience from checking this out locally was already a lot better than what we had before.
Also, in the future, if it's not too much trouble, could you include a screenshot of the UI in the PR description? It's not critical, but I definitely find stuff like that helpful as a reviewer.
|
Also, you're failing lint here, at least the code formatting check. To automatically reformat your code, you can run |
…, and update css class names
There was a problem hiding this comment.
Thanks for making these updates! This mostly looks good to me. I think one last functional request I'd make is to sort the articles by status, at least on initial page load. I'm thinking we should do the order "Scheduled", "Active", "Expired", though I think it could also make sense to do "Active", "Scheduled", "Expired". Either way, the expired articles really should go to the bottom of the list, because otherwise, as we accumulate more and more articles, the expired ones will take up more space and push down the other two status.
Also, it looks like you're still failing the lint checks. I think some of it should be automatically fixable by running Prettier, but the remaining ones you may need to fix up manually.
app/pages/EditBreakingNewsPage.tsx
Outdated
|
|
||
| sortedArticles.sort((a, b) => { | ||
| const statusOrder = ["Scheduled", "Active", "Expired"]; | ||
| const statusA = articleStatus(a).props.children; |
There was a problem hiding this comment.
Does this actually work? Even if it does, I don't think we should be trying to access the props on a nested React component like this. Instead, I'd factor this out into two separate pieces: 1) a pure function that just returns a status string, and 2) a React component that renders the badge icon.
app/pages/EditBreakingNewsPage.tsx
Outdated
| const onSave = (articleId: string) => { | ||
| const article = breakingNewsArticles.find( | ||
| ({ id }: any) => id === articleId | ||
| const articleStatus = (article: NewsArticle): JSX.Element | null => { |
There was a problem hiding this comment.
Related to my other comment, to turn this into a true React component, change the argument from just a plain article to an an object with an article property. Also, the convention for React components is to capitalize the first letter of the name, since React components were originally written only as classes, before they added function-based components.
| const articleStatus = (article: NewsArticle): JSX.Element | null => { | |
| const ArticleStatus = ({ article}: { article: NewsArticle}) => { |
(The return type can usually be omitted from React components as well)
|
Thanks again for working on this @GeorgeCloud! This will be greatly appreciated by the team! |
This ticket fixes issues with the edit page UI here is the ticket
We need to merge these API changes before this PR.