Session cover pages #54

Krejdom · 2017-03-02T15:29:47Z

created dirs and markdown files,
created link in lesson list of the run,
prepare for linking with navigation arrows

encukou · 2017-03-02T20:07:57Z

runs/2017/pyladies-brno-jaro-po/sessions/install/front.md

+
+## [Odkaz na feedback]
+
+[Odkaz na feedback]: http://s.quickmeme.com/img/a8/a8022006b463b5ed9be5a62f1bdbac43b4f3dbd5c6b3bb44707fe5f5e26635b0.jpg


https://gist.github.com/encukou/290a5b519656da51780be05f0e894b9c :)

encukou · 2017-03-02T20:12:17Z

naucse/routes.py

+                nxt_link = str(next)
+                nxt_title = next.title
+
+    return prv_link, prv_title, nxt_link, nxt_title


It would be nice to have a class to group (link, title). Something like a tooltip might be added in the future.
Could you use a namedtuple or SimpleNamespace instead of two variables for each link?

encukou · 2017-03-02T20:15:20Z

naucse/routes.py

    for prev, current, next in zip([None] + lessons,
                                   lessons,
                                   lessons[1:] + [None]):
        if current.slug == lesson.slug:
            if prev:
                prev = prev.index_page
+                prv_link = str(prev)


This shouldn't return the text representation of a page, but an URL. The text representation is a human-readable string that might easily change in the future.
Make prv_next_teller take the subpage_url function as argument if necessary.

encukou · 2017-03-02T20:17:39Z

naucse/templates/lesson.html

                <div class="col text-right">
-                    <a href="{{ lesson_url(nxt.lesson, page=nxt.slug) }}">{{ nxt.title }} →</a>
+                    <a href="{{ lesson_url(('/').join(nxt_link.split('/')[:2])) }}">{{ nxt_title }} →</a>


URL manipulation should be left to the URL converters and @app.route(). Avoid having logic involving / elsewhere – you're making too many assumptions about the structure of the object's str() representation.

encukou · 2017-03-02T20:20:52Z

naucse/templates/session.html

+                </div>
+            {% endif %}
+        </div>
+        {% endif %}


This should be made into a macro. Could you start a template file with utility macros? Docs are here.

encukou · 2017-03-02T20:23:24Z

naucse/routes.py

+
+    md_content = Markup(convert_markdown(content))
+
+    return render_template('session.html', content=md_content, page=page)


A separate render_session function is not necessary. Its functionality is not that well separated from what should be in session_page.

This is unlike session_template_or_404, which has a clear purpose.

It's also unlike render_page, which exists as a separate function because it's called for both the "just a lesson" URL and for lessons in a run – the common code is in a shared function rather than repeated. But render_session is only called from one place.

encukou · 2017-03-02T20:25:45Z

naucse/routes.py

+
+def session_template_or_404(run, session, page):
+    env = app.jinja_env.overlay(loader=session_template_loader)
+    run_link = str(run).split(" - ")[0]


Don't parse the string representation of objects – what if someone decides to make it more useful and puts more info into it?
Use the individual properties. (Look into Run.__str__ for their names.)

encukou · 2017-03-02T20:30:53Z

naucse/routes.py

+
+    def session_url(session):
+        """Link to the specific lesson."""
+        return url_for('session_page', run=run, session=session, page=page)


Where is this session_url function used?

encukou · 2017-03-05T20:29:16Z

Last time, I made fairly big changes to make the code look the way I wanted, to make everything ready in time. I'd like to avoid that from now on, and merge as soon as the content for current runs (PyLadies and Python's Libraries) is good to go. I've marked issues for that as blockers.

The pages that aren't linked from current runs are a playground now. Let's iterate on making it as good as it can be, but the iterations can take longer. I'll be happy to merge if non-blocker issues are remaining – that's why there's a red warning on the site, after all.

With that, please consider the points below. Set your priorities. And, don't make a change just because it's here – if you're not convinced, let's discuss.

Page issues

(Blocker) When a lesson has sub-pages, their prev/next links are a bit specific. Consider:
- The index of "Install Python" nas no next link; the next page depends on the OS, and is linked from the main text
- The page for Linux, then, links back to its previous subpage.
- This is all configured in info.yml and exposed in the model
Perhaps the model should be extended to work with the Arrow class (see below for notes on that). Perhaps you can find a better way to achieve this. But the way the arrows point needs to consider subpages.
The three links in the footer, (prev, up, next), won't show up properly on small screens when the titles are long. How can that be fixed? I recommend designing for small screens first, then adjusting for bigger ones if needed.
My idea was to make each lesson read like a book: the links at the bottom would take thee reader from an introduction, through the content, and finally to a summary. The front page would give the reader an overview of the section – to get them interested, to help them decide if the content is new for them, and to give links to related material. The back page would summarize the session, and prompt the reader to take a break and do some exercises. Without these pages being linked as if they were the first and last lesson, I don't think they are serving the purposes I had in mind as well as they should. We can talk about that, but even if you disagree:
It is not clear where to go next from the first page

Possible technical debt

Here are things that won't affect the result, but might make the code more pleasant to work with in the future.

Krejdom · 2017-03-10T15:31:11Z

The index of "Install Python" has no next link; the next page depends on the OS, and is linked from the main text

In this pull request it has the next link to install-editor. When you click on some link in this lesson, it's subpage. When you go through lessons with arrows, you look at index install page, choose subpage and install something or you realize you have already installed these, so you can directly continue to the next page.

The page for Linux, then, links back to its previous subpage.

Again, not in this pull request. Subpage links to the next lesson. Maybe the session link should be replaced with link to chapter (install).

The three links in the footer, (prev, up, next), won't show up properly on small screens when the titles are long. How can that be fixed? I recommend designing for small screens first, then adjusting for bigger ones if needed.

What you mean by "won't show up properly"? I don't see any problem. But if there is a serious problem on small screens, is can be replaced by just arrows ←↑ → there.

It is not clear where to go next from the first page

What you consider as the first page here?

There are three navigation links now.
Can code that's common to all three be combined?

Which common code?

encukou · 2017-03-14T12:54:05Z

The index of "Install Python" has no next link; the next page depends on the OS, and is linked from the main text

In this pull request it has the next link to install-editor. When you click on some link in this lesson, it's subpage. When you go through lessons with arrows, you look at index install page, choose subpage and install something or you realize you have already installed these, so you can directly continue to the next page.

OK, that does make sense here.

The page for Linux, then, links back to its previous subpage.

Again, not in this pull request. Subpage links to the next lesson. Maybe the session link should be replaced with link to chapter (install).

On "Install Python for Linux", the links should be:

back: "Install Python"
forward: the next lesson

The point is that the prv link should point to the index subpage, not to the previous lesson.

The three links in the footer, (prev, up, next), won't show up properly on small screens when the titles are long. How can that be fixed? I recommend designing for small screens first, then adjusting for bigger ones if needed.

What you mean by "won't show up properly"?

This:

I don't see any problem. But if there is a serious problem on small screens, is can be replaced by just arrows ←↑ → there.

If you can you make it work for all sizes, that would be great!
But it's not a blocker; it can be done after the backend is done.

It is not clear where to go next from the first page

What you consider as the first page here?

The cover page at the beginning of the session.

There are three navigation links now.
Can code that's common to all three be combined?

Which common code?

Mainly in the template.
Again, this is not a priority.

encukou · 2017-03-19T08:53:02Z

naucse/routes.py

+    if (page.subpages != None and page.slug in page.subpages.keys()
+    and "prev" in page.subpages[page.slug].keys() and "next" in page.subpages[page.slug].keys()) :
+        prev_segments = page.subpages[page.slug]["prev"].split(':')
+        next_segments = page.subpages[page.slug]["next"].split(':')


If I'm reading correctly, this way the title of the link can't have a colon in it. Is that a reasonable limitation?

The configuration already comes from YAML. Why invent a special format for this field?

encukou · 2017-03-19T08:54:30Z

naucse/routes.py

+            prev_segments.append("index")
+
+        kwargs['prv'] = models.Navigation(prev_segments[0], prev_segments[1], prev_segments[2])
+        kwargs['nxt'] = models.Navigation(next_segments[0], next_segments[1], next_segments[2])


All the code to create a Navigation is duplicated twice here. Is there a better place for initialization code to go?

encukou · 2017-03-19T08:56:51Z

naucse/models.py

+
+    Arguments:
+        title   the title of the link location
+        url     <lesson_type>/<lesson>


It's not really a URL.

[Ostrava] Feedback for classes

English review

Krejdom added 3 commits March 2, 2017 15:32

Create cover page for sessions.

c0682c9

Let prv_nxt_teller return lesson URL and title.

064c4d2

Fix typo.

00e8e52

encukou reviewed Mar 2, 2017

View reviewed changes

Krejdom added 5 commits March 3, 2017 08:45

Use slug instead of string parsing.

60d1c41

Add link to feedback.

fcb506d

Use namedtuple for prev and next page.

5763356

Add link to session from lesson.

a0ad2a0

Remove render_session function.

ff609ce

Krejdom added 6 commits March 9, 2017 11:20

Remove unused title_loader function.

d778cb9

Modify DocStrings.

f687549

Use the same template for lessons and sessions.

5606962

Introduce the Navigation class.

03b77f4

Merge origin changes.

94957c3

Remove link to session.

10a44b7

Krejdom added 4 commits March 16, 2017 10:58

Merge upstream changes.

2d4baba

Add sesson links to runs.

0c86ad4

Merge branch 'master' of https://github.com/pyvec/naucse.python.cz

ada1347

Redirect navigation on subpages.

19e9a3e

Krejdom assigned Krejdom and unassigned Krejdom Mar 17, 2017

encukou reviewed Mar 19, 2017

View reviewed changes

naucse/models.py

Arguments:

title the title of the link location

url <lesson_type>/<lesson>

Copy link

Member

encukou Mar 19, 2017

Choose a reason for hiding this comment

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

It's not really a URL.

Krejdom closed this Apr 7, 2017

frenzymadness added a commit to frenzymadness/naucse.python.cz that referenced this pull request Nov 27, 2019

Merge pull request pyvec#54 from frenzymadness/autumn2019

32ff178

[Ostrava] Feedback for classes

encukou pushed a commit that referenced this pull request Dec 16, 2019

Merge pull request #54 from rootli/patch-34

e805842

English review

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Session cover pages #54

Session cover pages #54

Uh oh!

Krejdom commented Mar 2, 2017 •

edited

Loading

Uh oh!

encukou Mar 2, 2017

Uh oh!

encukou Mar 2, 2017

Uh oh!

encukou Mar 2, 2017

Uh oh!

encukou Mar 2, 2017

Uh oh!

encukou Mar 2, 2017

Uh oh!

encukou Mar 2, 2017

Uh oh!

encukou Mar 2, 2017

Uh oh!

encukou Mar 2, 2017

Uh oh!

encukou commented Mar 5, 2017 •

edited by Krejdom

Loading

Uh oh!

Krejdom commented Mar 10, 2017

Uh oh!

encukou commented Mar 14, 2017

Uh oh!

encukou Mar 19, 2017

Uh oh!

encukou Mar 19, 2017

Uh oh!

encukou Mar 19, 2017

Uh oh!

Uh oh!


		## [Odkaz na feedback]

		[Odkaz na feedback]: http://s.quickmeme.com/img/a8/a8022006b463b5ed9be5a62f1bdbac43b4f3dbd5c6b3bb44707fe5f5e26635b0.jpg


		md_content = Markup(convert_markdown(content))

		return render_template('session.html', content=md_content, page=page)

Session cover pages #54

Session cover pages #54

Uh oh!

Conversation

Krejdom commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

encukou commented Mar 5, 2017 • edited by Krejdom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Page issues

Possible technical debt

Uh oh!

Krejdom commented Mar 10, 2017

Uh oh!

encukou commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Krejdom commented Mar 2, 2017 •

edited

Loading

encukou commented Mar 5, 2017 •

edited by Krejdom

Loading