Skip to content

Dewikification - Create app for resources + index page of resources#395

Merged
jerbob merged 15 commits into
python-discord:dewikificationfrom
ks129:resources-home
Oct 28, 2020
Merged

Dewikification - Create app for resources + index page of resources#395
jerbob merged 15 commits into
python-discord:dewikificationfrom
ks129:resources-home

Conversation

@ks129
Copy link
Copy Markdown
Contributor

@ks129 ks129 commented Sep 22, 2020

Created app for resources. Made index of it (where is all types of resources) looks exactly same than wiki version of it. Now this page is only clean HTML (with CSS included). Moved some blocks who defined custom background color these color definition to CSS file.

Fixes #387 .

@ks129 ks129 requested a review from a team as a code owner September 22, 2020 17:53
@ks129 ks129 requested review from manusaurio and scragly and removed request for a team September 22, 2020 17:53
@ghost ghost added the needs 2 approvals label Sep 22, 2020
from django.views import View


class ResourcesView(View):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you consider using a TemplateView here? It would do away with all of the boilerplate code:

class ResourcesView(TemplateView):
    template_name = "resources/resources.html"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Woah, I didn't know about TemplateView 😅 . Now this use this.

@ghost ghost added s: waiting for author Waiting for author to address a review or respond to a comment and removed needs 2 approvals labels Oct 4, 2020
@ghost ghost added needs 2 approvals and removed s: waiting for author Waiting for author to address a review or respond to a comment labels Oct 4, 2020
@ks129 ks129 requested a review from SebastiaanZ October 4, 2020 11:51
@lemonsaurus lemonsaurus linked an issue Oct 4, 2020 that may be closed by this pull request
Comment on lines +12 to +20
<section class="breadcrumb-section section">
<div class="container">
<nav class="breadcrumb is-pulled-left" aria-label="breadcrumbs">
<ul>
<li class="is-active"><a href="/resources">Resources</a></li>
</ul>
</nav>
</div>
</section>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Including breadcrumbs at the index feels redundant, especially since there's the "Resources" title right below. Does anyone else feel the same way? Is it better to keep it for consistency?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As other resources pages will have it too, I think it is better to leave it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The previous version removed/hid it because it was considered redundant and we'd have a page title, a breadcrumb and a content title all with the same word, which looked kinda weird. Take that how you will.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually I removed this breadcrumb now.

@ghost ghost removed the needs 1 approval label Oct 8, 2020
Comment thread pydis_site/apps/home/urls.py Outdated
Comment thread pydis_site/apps/resources/urls.py Outdated
<h1>Resources</h1>

<div class="tile is-ancestor">
<a class="tile is-parent" href="/articles/category/guides">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm aware this endpoint hasn't been implemented yet, but just making a note here so you're aware:

It's preferable to use django's {% url %} tag when referencing views, so we can reference it by namespace rather than the relative URL. When the articles endpoint exists we'll need to go back and use the url tag here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I this we should get this merged faster than content app one, as then I can make change there about it.

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.

Dewikification: Create a Django app for showing lists of resources.

6 participants