-
Notifications
You must be signed in to change notification settings - Fork 11
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
ADR list sortable and link to respective pages. #191
Conversation
PGaigole
commented
Jul 3, 2024
- The ADR's listing should be sortable
- The ADR listing should link to the ADR page which shows all the attributes of the ADR and the content
✅ Deploy Preview for axelerant-engg-handbook ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
@PGaigole, I see you are sorting by date and that's fine. But we could probably give some links to control the sorting (in a future issue).
More importantly here, let's show the metadata in the ADR view. Right now, only the markdown is rendered on the pages. Let's show the metadata from the ADR in a box in the beginning. I get that the markdown can duplicate some content and that is fine.
Edit: A "box" is just an idea. Feel free to design this in a way that makes sense.
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.
@PGaigole, I see the categories filter. This is great and thank you for your work here. We can talk about design later. The metadata display point is still open. Let's discuss that in another PR. Or please look at the comments here and see if you want to do this together here.
If you want to do it here, I had another idea to show the metadata. On regular pages, we have the table of contents on the right. We can use that area to show the metadata first and then the table of contents. The problem there is that the right area is hidden on narrow screens and we may have to do something about that.
if (category === 'All' || categories.includes(category)) { | ||
card.style.display = 'block'; | ||
} else { | ||
card.style.display = 'none'; | ||
} | ||
}); |
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.
How do you feel about replacing this with the ternary operator?
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.
if (category === 'All' || categories.includes(category)) { | |
card.style.display = 'block'; | |
} else { | |
card.style.display = 'none'; | |
} | |
}); | |
card.style.display = (category === 'All' || categories.includes(category)) ? 'block' : 'none'; | |
}); |
if (buttonText === category) { | ||
button.classList.add('active'); | ||
} else if (category === 'All' && buttonText === 'All') { | ||
button.classList.add('active'); | ||
} else { | ||
button.classList.remove('active'); | ||
} |
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.
if (buttonText === category) { | |
button.classList.add('active'); | |
} else if (category === 'All' && buttonText === 'All') { | |
button.classList.add('active'); | |
} else { | |
button.classList.remove('active'); | |
} | |
button.classList.remove('active'); | |
if (buttonText === category) { | |
button.classList.add('active'); | |
} |