Skip to content
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

Closed
wants to merge 3 commits into from
Closed

Conversation

PGaigole
Copy link

@PGaigole 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

@PGaigole PGaigole requested a review from hussainweb July 3, 2024 09:10
Copy link

netlify bot commented Jul 3, 2024

Deploy Preview for axelerant-engg-handbook ready!

Name Link
🔨 Latest commit ac3cd11
🔍 Latest deploy log https://app.netlify.com/sites/axelerant-engg-handbook/deploys/66853eb301afc500087d79c6
😎 Deploy Preview https://deploy-preview-191--axelerant-engg-handbook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@hussainweb hussainweb left a 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.

@PGaigole PGaigole linked an issue Jul 3, 2024 that may be closed by this pull request
3 tasks
Copy link
Member

@hussainweb hussainweb left a 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.

Comment on lines +7 to +12
if (category === 'All' || categories.includes(category)) {
card.style.display = 'block';
} else {
card.style.display = 'none';
}
});
Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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';
});

Comment on lines +16 to +22
if (buttonText === category) {
button.classList.add('active');
} else if (category === 'All' && buttonText === 'All') {
button.classList.add('active');
} else {
button.classList.remove('active');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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');
}

@PGaigole PGaigole closed this Jul 5, 2024
@hussainweb hussainweb mentioned this pull request Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show a listing of ADR's
2 participants