Follow-up to #2804551: Display mail attachments
Problem/Motivation
#2804587: Create mail display mock-ups provides a set of mockups for the mail display/Inmail message element.
Proposed resolution
Let's start with providing markup implementation for it. That means updating the relevant Twig template, providing semantic HTML markup and corresponding CSS.
Some parts are not code-ready yet. For instance, the "unknown" part, proper body display etc. Anyway, we should provide the markup for those cases (since we have it in available in variables) too, and later (in other issues) implement the logic.
* Keep in mind to post the screenshot when you provide a patch.
Remaining tasks
User interface changes
Email display implementation.
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#32 | interdiff-2820004-30-32.txt | 884 bytes | johnchque |
#32 | provide_markup_for_the-2820004-32.patch | 8.71 KB | johnchque |
| |||
#30 | interdiff-2820004-29-30.txt | 3.5 KB | johnchque |
#30 | provide_markup_for_the-2820004-30.patch | 8.9 KB | johnchque |
| |||
#29 | interdiff-2820004-27-29.txt | 279 bytes | ModernMantra |
Comments
Comment #2
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedThis is related to #2819981: Use the same template for both sorts of Inmail messages, and we should probably wait for it to be committed.
Comment #3
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedUpdating the status since #2819981: Use the same template for both sorts of Inmail messages was committed.
Comment #4
johnchqueFirst, early try. Added css, divided for each view mode.
Teaser:
Full:
Comment #6
miro_dietikerThx for helping to move forward.
We need much less boldness in the teaser.
To make a difference between the subject and the body, you can grey out the body text a bit. Google also does it this way. And i think we can even make it a bit smaller.
Comment #7
johnchqueMight be my installation but when changing the font weight from 0 to 500 the change is none.
Comment #9
miro_dietikerThis bracket needs encoding!
Not sure we really need the colons? With labels we usually try to avoid colons when possible.
This is footer, not header.
And yeah, you still display them bold. Why not simply regular?
Please consider my proposal to always print the semantic labels and hide them in CSS.
Why is the attachment label and the "all header" different indicated?
Can we quickly make this "All header" thing collapsed, reduced? It steals so much attention. We could also simply hide it in CSS and make it visible (CSS click handler) in a follow-up again.
I'm highly confused that the mail body is also present there. It should be headers only.
And the header should display preformatted (pre tags). A real line should stay a line.
Comment #10
johnchqueNow should be better. Also hidden the all headers by default. :)
Comment #12
johnchqueSorry, fixed tests and fixed indentation. :)
Comment #13
miro_dietikerI don't know of any other component that offers a CSS file per view mode. This contains many risks, see below.
Super dangerous and bogus. The same class receives different definitions depending if it is a teaser display or a full one.
That's why these things should stay in one file per component. And that's why they need to be fully qualified.
If some definition in CSS only applies the teaser, you need to prefix it with that selector too.
Comment #14
johnchqueSorry, true. Now all the css is in just one file. Added prefix for teaser elements.
Comment #15
pivica CreditAttribution: pivica at MD Systems GmbH commentedThis comment is originally writen for as a feedback for #12, sorry i don't have time to preview patch in #14, but i guess still a lot of stuff applies for new patch also.
There is lots of stuff here that can/should be improved for frontend. Here are couple of things for now.
Do note that none of this code is tested - and there are probably errros ;)
Also sorry for a bigger comment, i've tried to explain more bigger picture and not just specific stuff related to this ;)
BEM syntax
D8 themes are using BEM syntax for naming convention.
For example instead of
If we apply correct BEM conventions we could have something like
Note the modifier variation for original inmail--view-mode-teaser. I've used inmail-message--teaser, but its also perfectly valid to use inmail-message--view-mode-teaser or inmail-message-view-mode--teaser. It's more a personal preference, generally i like to describe the class name with the less characters possible and in this case just inmail-message--teaser is probably enough.
Code re-usability
Template level
All frontend stuff starts from template and it's important to try as much as possible to write clean and well structured HTML because this will affect later CSS rules and JS code.
First thing, i know that its very tempting and easy to use base level if command and completly separate different view modes, in this case teaser and full view modes. However the problem is that with this approach developer is less enforced to write reusable code because its easy to just write completely different stuff for each view mode. So i would always try to start with only one template file and one structure that covers all needed cases.
Later if template code starts to be more complex and you are starting to get too many sub-component level if commands then it make sense to separate stuff but still try to reuse code - for example have base template inmail-message.html.twig and then use twig blocks and extends for inmail-message--teaser.html.twig. Check http://twig.sensiolabs.org/doc/tags/extends.html and https://ffwagency.com/blog/power-of-extending-twig-templates about this.
Here maybe something like this could work (mix of both template extend and if statements just for example - for sure you could define block body_content also and override it in teaser:
inmail-message.html.twig
inmail-message--teaser.html.twig
Couple of notes
- For sure for this to work we need additional template suggestions based on view mode.
- Converted 'inmail-message--header-label' div to just label tag.
- Instead of inmail-message__body-plain use inmail-message__body__content - you want to do variations in CSS with selector when possible and not pollute HTML markup with various CSS classes which are used in just some specific cases.
- For separator like > and | consider using CSS before/after pseudo elements for this because this are more visual separators.
This is just suggestion and CSS rules for this needs tweaking.
- Consider using label tag for labels without any CSS class.
CSS level
With template in place we can now write more efficent CSS in only one file
Other notes
1.
Checking html and i dont see that this CSS selector is used anywhere?
If we are covering webkit with prefixes then we shoudd do also prefixes then check should we use
-webkit-flex-wrap: wrap; /* Safari 6.1+ */
2.
Code intend for everything in Drupal, including twig is 2 spaces. Currently twig template is using 4 spaces.
Comment #16
johnchqueThanks for the feedback, will upload a new patch soon.
Comment #17
pivica CreditAttribution: pivica at MD Systems GmbH commented> Thanks for the feedback, will upload a new patch soon.
Cool, just be sure to check everything, none of the code that is written is tested so for sure there are bugs.
Comment #18
miro_dietiker:-)
Comment #19
johnchqueInterdiff too big. I wasn't able to make the extends work, not really sure why. Also, I was wondering if it might be better (if extends works) to create a "base" twig that defines the main areas of the mail (header/body/footer) and then override blocks on different twig templates to add the content that is needed. We would still use the selectors provided by @pivica and thus the code would be even more readable. This is an early patch with the new changes. Full view mode works nice but still thinking that creating a "base", "teaser", "full" twig templates is IMHO better.
Comment #20
miro_dietikerTo make extends / suggestions work, checkout:
block_theme_suggestions_block()
With twig files like:
core/themes/stable/templates/block/block--system-branding-block.html.twig
core/themes/stable/templates/block/block.html.twig
Please keep refactoring limited in a way that this issue can be completed quickly. Otherwise it blocks all other issues as it creates more and more overlap.
Comment #21
johnchqueAdded everything in a twig, changed selectors, added css, addressed @todo in twig file. :)
Comment #23
johnchqueRemoving the @todo change. That better to do it in another issue. :)
Comment #24
miro_dietikerTeaser and full HTML could share the same markup and use CSS to hide labels and other parts.
We now have a 100% split inside a single file, that's still full duplication.
As ivica pointed out, the arrow and other indications could be easily done with CSS ::before and ::after and that would improve semantics.
Keep in mind that with making each address an element, each of them will have another div/span container. Keeping the html artificially flat in the heaser doesn't help.
We can move some improvements on that in a follow-up though. Let's complete this fast as it overlaps with lots of issues improving header and body part.
I think the footer should be on same level as header and the body should be a separate container from the footer area.
Comment #25
johnchqueOhhhh true, this will be better then, the display is the same as the last screenshot. :)
Comment #26
miro_dietikerWhy not add the class to the header element?
This should definitively only be output in full view mode.
Why not add the class to the footer element?
Comment #27
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedMade small changes as suggested in comment #26
Comment #28
johnchqueYou still need to fix the indentation.
And then you can remove this.
Comment #29
ModernMantra CreditAttribution: ModernMantra at MD Systems GmbH for MD Systems GmbH commentedRemoved block of code as suggested in comment #28.
Comment #30
johnchqueSorry, this have been taking too much time, this should be OK. btw Thanks @ModernMantra for the patch :)
Comment #31
pivica CreditAttribution: pivica at MD Systems GmbH commentedInstead of all this can we just write next stuff?
Just checking can we maybe just use margin-bottom: 0.1em if there is no need to override other values?
Similar to previous one check can we just do
Usually this pattern is used because it is easier to override this kind of CSS rules then the one that use all values and shorter syntax. But if wee need to override left and right and set it to 0 then it is fine.
Comment #32
johnchqueThank you so much, yes, makes so much sense. Addressing changes on comment above. :)
Comment #33
miro_dietikerNow this reads nice.
Committed, i guess lots of issues to catch up with the markup.