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

Flux Website Resource Page Update #1693

Closed
wants to merge 14 commits into from

Conversation

thisisobate
Copy link
Contributor

@thisisobate thisisobate commented Oct 26, 2023

Fixes cncf/techdocs#192
Todos

  • Create UI for filter
  • Create search index for filter
  • Create logic for filter
  • Make mobile responsive
  • Test if everything works as expected
  • Clean code
  • Modify thumbnail logic to support article urls

Signed-off-by: thisisobate <obasiuche62@gmail.com>
@kingdonb
Copy link
Member

Thank you for getting started on this! I have taken a screenshot of the progress as I found it so far, then it will be easier for reviewers. 🚀

Screenshot 2023-10-26 at 11 34 04 AM

Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
@thisisobate thisisobate marked this pull request as ready for review November 17, 2023 14:36
@thisisobate
Copy link
Contributor Author

@kingdonb Please feel free to review

Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
@thisisobate
Copy link
Contributor Author

@juozasg

@kingdonb
Copy link
Member

Thanks! I have this on my radar, I see you're ready! We've got the KubeCon Paris CFP deadline coming up in two days, and then we'll be more available for PR reviews. It looks like you have completed the work based on the checklist ✅

Assigning myself so this gets kept track of!

@kingdonb kingdonb self-assigned this Nov 24, 2023
@kingdonb
Copy link
Member

Adding some review comments with screenshots to highlight the work I see that is new, please feel free to add comments that detail any parts I should review or might have missed!

Screenshot 2023-11-24 at 10 10 26 AM

The "video" tags are new - the date in the corner is nice, but sometimes overlaps the text!

Screenshot 2023-11-24 at 10 12 50 AM

The Article type entry is new. We have added a link to the Weaveworks blog where an article has been published 🎉 this is what we wanted, perfect. The advance or development here is that we can republish articles from an alternative source, which is exactly what we asked for in SD-1944. Bravo, thank you

The UI chrome can sometimes float above the header bar! That's less than ideal, but I don't know if that's something you can try to fix before we merge this? I think this is ready for review anyway. Look here in the upper left:

Screenshot 2023-11-24 at 10 13 10 AM

You can filter by content type, to show only articles. The one article we publish first being a link to the Weaveworks website is probably fine. That's neither here nor there

Screenshot 2023-11-24 at 10 15 02 AM

This looks good! Can we have some feedback from maintainers, once these issues are addressed does this look like something we can merge, (or do we need to add more external articles first?)

Signed-off-by: thisisobate <obasiuche62@gmail.com>
@thisisobate
Copy link
Contributor Author

@kingdonb I have fixed all the issues you mentioned above.
Please kindly take a look.
Thanks!

@thisisobate
Copy link
Contributor Author

do we need to add more external articles first?

I think this can be done by the community as a follow-up PR.

cc: @kingdonb

@kingdonb
Copy link
Member

We've just had another chance to review this, it looks like all the corrections you made are good! I'm happy to approve this.

(Will be rebasing before merge)

@xunholy
Copy link
Member

xunholy commented Dec 12, 2023

LGTM - Only tiny thing I'd mention is handling EOL at EOF which can be easily fixed in your IDE settings.

@kingdonb
Copy link
Member

I'm going to reopen this, I cannot merge it by myself and I cannot edit your branch, but I've rebased the commits and merged it with #1748 that created the conflict

@kingdonb
Copy link
Member

Reopened as #1749

(Will be likely merged as soon as I have an approving review from another maintainer with write access!)

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.

CNCFSD-1944 Flux Website Resources Page update
3 participants