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

refactors + adds sidebar to account settings #6427

Merged
merged 3 commits into from
Apr 20, 2022
Merged

Conversation

mekarpeles
Copy link
Member

@mekarpeles mekarpeles commented Apr 14, 2022

Refactors the sidebar out of the account/books.html template and plugins/mybooks.py so that it can be added/used w/ the /account settings page.

Technical

NB: The old sidebar.html code is remains / is duplicated by this PR and needs to be consolidated + cleaned up in account/books.html template

Testing

Screenshot

localhost_8080_account

Stakeholders

@jimchamp @cdrini

@mekarpeles
Copy link
Member Author

mekarpeles commented Apr 14, 2022

After this refactor PR lands, @tangym27 can follow the example of the Settings Page changes in this PR to also add this new sidebar to the patron Profile Page whose code lives in https://github.com/internetarchive/openlibrary/blob/master/openlibrary/templates/type/user/view.html

@cdrini cdrini marked this pull request as draft April 14, 2022 20:02
@jimchamp
Copy link
Collaborator

I'm reviewing this now. Why was it converted to a draft?

@cdrini cdrini mentioned this pull request Apr 14, 2022
4 tasks
@mekarpeles mekarpeles marked this pull request as ready for review April 15, 2022 16:49
@mekarpeles
Copy link
Member Author

It was originally converted to draft because the code in books.html remained duplicated and has since been migrated into sidebar.html

@mekarpeles mekarpeles added the Priority: 1 Do this week, receiving emails, time sensitive, . [managed] label Apr 18, 2022
@mekarpeles mekarpeles added On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing Module: My Account Page labels Apr 20, 2022
@mekarpeles
Copy link
Member Author

Just worked through a final bug w/ @jimchamp re: selected List pages (by pulling the selected list key from the ctx.path current url)

Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

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

Lgtm! Tested that sidebar bug has been resolved on testing.

$for lst in lists:
$ list_id = lst.key.split('/')[-1]
$ class_list = ''
$if lst == items:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The sidebar is not rendering because it is missing items, which is only used here to determine if the current list is selected:

items_missing_error

In the list context, items is the list itself, which is probably overkill to send to the sidebar template. Maybe sending the list ID as the key would work better:

Suggested change
$if lst == items:
$if list_id == key:

Copy link
Collaborator

Choose a reason for hiding this comment

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

As an aside, it was wrong of me to name that parameter key, as it doesn't fit the pattern of OL keys. Maybe selection is better? Anyway, this doesn't need to be changed now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anyway, you'll have to set the key for lists here:

elif self.key == 'list':
data = list

Adding the following at the end of that block worked for me:
self.key = list.key.split('/')[-1]

@jimchamp jimchamp merged commit 429f978 into master Apr 20, 2022
@cdrini cdrini deleted the refactor-sidebar branch April 20, 2022 18:31
@jimchamp jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 1 Do this week, receiving emails, time sensitive, . [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants