Needs review
Project:
Lost & found issues
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
15 Jun 2017 at 11:14 UTC
Updated:
5 Dec 2017 at 10:56 UTC
Jump to comment: Most recent, Most recent file
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | create_a_twig_template-2886417-8.patch | 3.45 KB | sorabh.v6 |
| #8 | create_a_twig_template-2886417-8-interdiff.txt | 3.95 KB | sorabh.v6 |
| #4 | create_a_twig_template-2886417-4.patch | 6.61 KB | sorabh.v6 |
| #4 | create_a_twig_template-2886417-4-interdiff.txt | 7.28 KB | sorabh.v6 |
| #2 | create_a_twig_template-2886417-2.patch | 6.95 KB | sorabh.v6 |
Comments
Comment #2
sorabh.v6Patch created for twig templating. Please review.
Comment #3
joshmillerSpace between class and bracket is missing.
Spacing of declarations and values is wrong.
Too general. This would affect all elements on the entire website, put a wrapper class on your popup and use that to specifically call elements within it.
Class names need to be more descriptive. Instead of "name" maybe "product-name" ... instead of "right-div" go with what it contains "order-subtotal"
.button is not great as a class name, but if you have shared characteristics between all buttons (like: border & display) then create a generic class "button" that sets up one of the buttons, and extend that class as needed. For example, you create a generic button
<a href="#" class="button">Test</a>and it has rounded corners and 10px padding. Next, you need a blue button, you can create this:<a href="#" class="button button-blue">Test Blue</a>and define the blue button to have color: blue;. Using two classes like this works well and allows for inheritance to keep your CSS clean.Nope. We just want to render the product using a display mode. Using the display mode, the user can choose which fields get displayed and use twig to further create their perfect layout. We just want to render the product. We need a new display mode called "Add to Cart Confirmation" for product variations.
Too much formatting going on here. Remember, we want the designers to have as much control as possible. We can give them a product and an order. The rest of this can be determined using twig.
We want to remove the debug code before committing.
If you are depending on bootstrap then you should write that in the README.md. Hint: We don't want to depend on a specific kind of theme. This should work on any number of commerce installations, including those that use none bootstrap CSS.
Really? Use class names to call out what you're trying to do, using tag names as class names is incomprehensible and will fail to help you, long term. http://bdavidxyz.com/blog/how-to-name-css-classes/
An
<a>tag that is wrapping<div>tags??? Inline elements CANNOT and SHOULD NEVER wrap block elements. https://www.w3schools.com/html/html_blocks.aspComment #4
sorabh.v6Interdiff and Patchfile created for the CSS changes, changes for product variation view mode are yet to be implemented. Please review.
Comment #5
shabana.navas commentedExtra space here
Extra space here
Comment #6
shabana.navas commentedThe class names seem peculiar. Can't you just target the span class and set the text-transform property to uppercase? Also, what's 'subm'?
Comment #7
shabana.navas commentedIf I am understanding this correctly, I think what should be here is actually just a rendering of the product display mode instead of displaying each product item like the image, sku, total, etc. So, anyone can come in and override it.
I believe something like this is what is expected, except, that it should render the product entity:
$nid = 1;
$entity_type = 'node';
$view_mode = 'teaser';
$view_builder = \Drupal::entityTypeManager()->getViewBuilder($entity_type);
$storage = \Drupal::entityTypeManager()->getStorage($entity_type);
$node = $storage->load($nid);
$build = $view_builder->view($node, $view_mode);
$output = render($build);
?>
Comment #8
sorabh.v6Created new interdiff and patchfile. Removed css and html from twig template. Need to work on it again. As html for product variation is coming from drupal itself. So, I will work on new css styling. Please review.
Comment #9
sorabh.v6Comment #10
sorabh.v6