-
Notifications
You must be signed in to change notification settings - Fork 6
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
💄 [#1021] Make entire Product card clickable #457
Conversation
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.
- The card is still not entirely clickable (areas 1,3,5 are clickable, 2 and 4 are not)
- If we put the button inside the
card__body
it should be flex or grid (it doesn't have the right position)
Generally, I am not the best person to review frontend, but I think we should see this from another view. Maybe change the whole card in order to achieve a clickable card and not specifically the body (if there is a link). Bart can tell us his opinion as well.
<div class="card__content"> | ||
{% endif %} | ||
<h2 class="h2">{% link url text=title %}</h2> | ||
{% load card_tags icon_tags link_tags i18n grid_tags thumbnail %} {% render_card |
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.
This change in the code linting creates a problem in rendering the page (template syntax error). The linebreaks in the template tags breaks the template.
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.
@jiromaykin This is the same thing we had yesterday at your desk with the if/else statement. Breaking template tags insde the tag delimiters is a bity fishy (not as robust as html itself)
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.
Yes, I had not seen this break yet; also it seems like my IDE is a bit fishy with templates/html.
I think it would be a good idea to collect a little overview of the places in the site where these card CSS/template tags are re-used so we can decide if these changes on the card body would affect things we're not aware of. Also I don't know what is best practice on 'the whole card clickable', if it should include the background or just all elements. |
Also if this is a bit complicated for comments we can do a quick call next time when we all work, it is easier to talk about things verbally. |
Yes, thank you @vaszig and @Bartvaderkin - I was hoping to create a relatively easy "CSS only" solution, but this won't work for all situations, so next step for me is to learn how Django templates work, because I will definitely need to change the structure of the Cards. +Also it seems the design is suggesting all arrows need to always be all the way at the bottom of each card (instead of moving along with content) but we will need to check this with @alextreme and Roxanne. |
Also earlier I didn't configure Jiro's Pycharm to use Django templates (it picked Jinja) so it didn't catch and highlight some of the formatting issues we encountered. This should work better now. |
282b167
to
7c711fb
Compare
Overview of cards on frontend: |
Codecov Report
@@ Coverage Diff @@
## develop #457 +/- ##
===========================================
- Coverage 96.48% 96.46% -0.02%
===========================================
Files 527 538 +11
Lines 19003 19204 +201
===========================================
+ Hits 18335 18526 +191
- Misses 668 678 +10
... and 31 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
382d076
to
ca41978
Compare
8e4957f
to
ab4226b
Compare
0dee2dc
to
e711ea6
Compare
Closing this older messy branch for now, and starting a new one here that is more up to date: #554 |
See #453
and: #454
these need to become fully clickable:
Note for accessibility: "... Decorative images don’t add information to the content of a page. For example, the information provided by the image might already be given using adjacent text, or the image might be included to make the website more visually attractive. In these cases, a null (empty) alt text should be provided (alt="") so that they can be ignored by assistive technologies, such as screen readers. ..."