Closed (fixed)
Project:
Honey
Version:
1.0.x-dev
Component:
Design
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
24 Jun 2020 at 23:48 UTC
Updated:
24 Aug 2020 at 17:59 UTC
Jump to comment: Most recent, Most recent file
I've committed a few things to this theme, so that at least it has a layout and some colors.
However, it's extremely ugly. It would be nice if someone could make it nicer
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | breadcrumb.png | 3.42 KB | jhodgdon |
| #19 | headerarea.png | 8.98 KB | jhodgdon |
| #16 | Web 1920 – 1.png | 273.96 KB | finnsky |
| #7 | 3154671-1.patch | 39.6 KB | finnsky |
| #6 | screenshot.png | 118.67 KB | finnsky |
Comments
Comment #2
jhodgdonAs a note, the color scheme for the Farmer's Market scenario in the User Guide is here:
https://git.drupalcode.org/project/user_guide/-/blob/8.x-8.x/assets/colo...
Comment #3
kostyashupenkoHello there
This is what i get under D9.1
Would be nice to summarize what exactly should be done here and how far we have to investigate markup and styles:
1. Should we override templates? (I mean is it supposed to be fully based on stable or we can use overrides?)
2. Should we use postcss?
3. Is js supposed to be written for things?
4. Accessibility things?
5. etc..
Or it is supposed to have just common styles?
What is the goal of this theme and which content is supposed to be there?
Thanks
Comment #4
kostyashupenkoComment #5
finnsky commentedComment #6
finnsky commentedThis patch contains mix of bartik(color module) and claro(css variables + build structure).
For postcss theme uses drupal core build scripts.
So for development needs theme should be putted to /core/themes/ directory.
Run `yarn && yarn build` in /core/ dir.
Now colors works and --variables works same way as in core.
Added simple logo and screenshot. Customized fonts and some typography
Suggested color palette changed a bit because all colors should be different imo to avoid conflicts.
It was easier for me to remove all initial work and start from scratch with variables and colors.
In case of questions catch me in drupal.slack.com - finnsky
i will continue to work on it after WE i guess
Comment #7
finnsky commentedComment #8
finnsky commentedAlso this is base of second color scheme i will apply later.
https://www.color-hex.com/color-palette/19739
Comment #9
jhodgdonThat is great -- thank you very much!
I think the patch will need a bit of adjustment for docs, no newline at end of file, etc., but I love that it is working and nice looking, and you got it working with the Color module too. The docs on how to use the Color module in Drupal were for Drupal 7 -- we really need a page in the Drupal 8/9 theming guide on how to do that.
As far as the purpose of this theme is concerned, see the project page
https://www.drupal.org/project/honey
I'm not really expecting this theme to be used by people for real sites. We just needed a theme that we could maintain and keep available for reference in the User Guide. But we wanted it to be at least pleasant to look at and usable, and probably as a model for how to create a child theme.
Comment #10
jhodgdonComment #11
jhodgdonI've taken a bit more of a look at this patch, and there are a few things that need attention and/or answers (I haven't actually installed it, just looking at the files in the patch):
a) The .css files have a note at the top referencing this change record:
https://www.drupal.org/node/2815083
However, that one is about JS using Es6. They should be referencing this change record about PostCSS instead:
https://www.drupal.org/node/3084859
If those files were copied directly from Drupal Core, we need to create an issue in the Drupal Core project to get that changed, because it's definitely wrong.
b) Actually rather than those comments... Since this is a separate contributed project, I don't think it's appropriate to reference a Drupal Core change record at all. What we should do instead is make a README.txt file that explains how to build the CSS and JS files from the source, and reference them.
c) Do we really want to use a font that is hosted externally? I am not sure I'm all that comfortable with that idea.
d) Are all of the other fonts that are referenced in the variables.pcss.css file available on most/all platforms and browsers?
e) I don't see any media queries in the CSS, aside from the print media. Is this theme mobile-friendly?
f) There's at least one file with
in the patch.
g) We should not have @todo notes from Drupal Core in our files -- typography.pcss.css
h) Typo in the variables.pcss.css file:
"ands"
Also in that file, the end-of-line comments are weird, such as
They all end up printing out at the top of every .css output file. ?!? Probably would be better to get rid of them, or make them all into one comment.
Comment #12
finnsky commentedthis is generated by core, i think that this issue already exists.
we may add own build tools based on core but i think it will be faster(safer) if we will use core tools as is with README of course.
yes
it was faster action for mvp. also it could be some system font. roboto was good secondary font. in my mind at least.
if it will be roboto or other googlefont we will copy files in theme /fonts dir
I think yes but better to research better later
Not yet, i prefer to add media queries based on honey.breakpoints.yml which isn't added yet
Same core generation herritage.
Evil copypaste. and it is obvious will be changed in future work.
Anyway thanks for quick review. I hope i will continue soon
Comment #13
jhodgdonThanks for the quick response! We should create a core issue to fix that bug. I totally agree about using the Core tools for building JS and CSS.
Regarding the font, we have to be careful about including files from other projects in ours, due to licensing differences. Here are the policies:
https://www.drupal.org/node/422996
https://www.drupal.org/node/2947530
https://www.drupal.org/node/1475972
I think I would rather either just use generic generally-available fonts or fonts we are able to include in our distribution of this theme, rather than referencing an external URL. This font is I think licensed under an Apache license, so we cannot include it directly. So... we can leave the external URL I guess, as long as there are fallbacks (which there are).
Comment #14
jhodgdonCreated the core issue about the wrong headers:
#3155159: Fix notice at top of generated CSS files from PCSS
Comment #15
jhodgdonSee also #3147404: Replace mentions of Mayo theme in User Guide for more about why we are creating this theme.
Also @finnsky, are you interested in possibly co-maintaining this theme?
Comment #16
finnsky commentedok about co-maintaining.
minimal desktop for secondary colors scheme.
Comment #17
kostyashupenkoHello, is slack chat created for the reference to this theme?
Comment #18
jhodgdonFor anyone following this issue, we are discussing design etc. on the #honey channel in Drupal Slack. Thanks @finnsky for creating it!
Comment #19
jhodgdonI'm running the issue branch as of the latest commit, and I found a contrast problem... I have the main menu in the header area, and the links are nearly impossible to see. They seem to be
foreground: #534400
background: #712c1e
Screenshot attached.
Comment #20
jhodgdonAnother problem: the breadcrumbs are an OL list that is showing up numbered.
Comment #21
jhodgdonAt this point, I think you've taken care of the "make this theme less ugly" issue. I'll create separate issues for other problems I notice. Thanks a bunch! I've merged this issue branch into the main branch.
Comment #22
jhodgdonI'm also going to make a 1.0 release of this theme.