-
Notifications
You must be signed in to change notification settings - Fork 56
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
Quick: Fix Grid Container get_short_description for Nested Tuples Setting #145
Quick: Fix Grid Container get_short_description for Nested Tuples Setting #145
Conversation
Codecov Report
@@ Coverage Diff @@
## master #145 +/- ##
=========================================
Coverage ? 98.93%
=========================================
Files ? 71
Lines ? 1225
Branches ? 160
=========================================
Hits ? 1212
Misses ? 8
Partials ? 5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
9907b0a
to
e691c87
Compare
0847a03
to
f0c1a9f
Compare
Also updated fallback choices, default value, and README sample values.
f0c1a9f
to
21db696
Compare
Hello @tacc-wbomar, Looks good, everything worked for me! 😄 |
…ortcut: commit 997e3c5 Merge: a4e636f bcd8359 Author: Wesley Bomar <wbomar@tacc.utexas.edu> Date: Tue Jan 18 21:51:04 2022 -0600 Merge branch 'main' into task/FP-1318-section-pattern-as-container-plugin-shortcut commit a4e636f Author: Wesley Bomar <wbomar@tacc.utexas.edu> Date: Fri Jan 14 18:55:45 2022 -0600 FP-1318: Create Section Pattern via BS4 Container Requires django-cms/djangocms-bootstrap4#145
Thank you for the review, @crydotsnake. How can I accurately describe the status of this PR to dependent parties? My Effort to Determine StatusI reviewed a contribution doc that mentions "Status", but I don't see on this PR any of the labels it describes. |
If the PR need be cleaned up anyway before merge, feel free to do it or explain the change for me to do. (I probably Python and Django'd my way around like a child, so I don't mind direction.) |
…sted-tuple-choices
I do not know how to write test cases in Python, Django, DjangoCMS. I am disinclined to learn, because I cannot even run the existing test cases. I have followed the instructions. I get the console error test case run failure console log
|
To the PR description, I have added sections: "Known Issues", (manual) "Testing" steps, a "Screenshot". |
@fsbraun were you going to look over this? |
...tstrap4/contrib/bootstrap4_grid/migrations/0005_change_container_type_choices_and_default.py
Outdated
Show resolved
Hide resolved
@wesleyboar @marksweb Sorry, I had the review done put left it pending. So I saw it but you couldn't. Should be fixed now! |
- do not change default value - thus no need for migration
I remember having problems with testing, too. Can you try to add As a thought starter, here's how I tested |
any 2nd tuple was never read cuz `return None` if first element no match
I can now run test cases. The problem was that I had not installed Django nor DjangoCMS. I have opened a PR with a change to the README (#152) that may help others. |
I've fixed the tests in master, but can't currently update this PR with the latest changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!! Thanks very much!
…sted-tuple-choices
Yes, thank you @marksweb. Thanks all of you working on Django. I did not have time to get back to this. How anyone does open source and holds a job is astounding. I used the "Sync fork" ▸ "Update branch" button in GitHub which looks to have also merged the upstream |
@wesleyboar Do not worry about the failing tests. Once #156 is merged you can sync your fork and we can merge this PR! |
Thank you. I was. I stop spelunking now. |
@wesleyboar Can you try update branch once more? |
To Do
Done
get_short_description
value is inaccurate (see screenshot). (4ba6861, 99dcc07).Description
Fix short description for Bootstrap4 Container if user choses container type from nested tuples in
DJANGOCMS_BOOTSTRAP4_GRID_CONTAINERS
setting.** An
<optgroup>
is available in "Container Type"<select>
field whenDJANGOCMS_BOOTSTRAP4_GRID_CONTAINERS
has deeply-nested tuples. But were user to chose such an option, thenget_short_descritpion
would return empty string.Related resources
Checklist
master
Slack to find a “pr review buddy” who is going to review my pull request.
Testing
wesleyboar:quick/grid-container-support-nested-tuple-choices
poc/djangocms-bootstrap4-pull-145
(see diff).12container
container-fluid
container-yours
Screenshots
Footnotes
Added
from django.utils.translation import gettext_lazy as _
toyour-app/settings.py
. ↩Added sample DJANGOCMS_BOOTSTRAP4_GRID_CONTAINERS to
your-app/settings.py
. ↩