Currently the design of the checkboxes components differs between the one used in Project Browser and the one outlined in the Drupal Design system.
Explanation to the first screenshot. the first checkbox is unchecked in both windows the second is set to :hover in the devtools, the third is checked and the fourth checked and set to :hover. the four available states. In project browser the hover states are missing. then the checkboxes are too small and the markup differs as well. semantically the variant used for claro seems more clear by having an input element (checkbox) with a corresponding label. in the current project browser iteration the input element is wrapped in its label. and in claro the checkmark is created by a svg, how it is done in project browser i am not sure but haven't digged.
- unchecked checkboxes should have a width and height of 1.125rem (18px) with a border radius of 2px
- the border of unchecked checkboxes is 1px solid #919297 the color on hover is #232429
- with a boxshadow of 0 0 0 4px transparent while on hover inset 0 0 0 1px #232429
- checked checkboxes should have a width and height of 1.125rem (18px) as well with a border radius of 2px
- the icons should have a width and height of 0.75rem (12px) - the svg icon has 1rem
- the border and background color is #003ecc on hover it becomes #232429
- labels should have a fontsize of 0.889em
not sure if i was able to distill everything necessary to provide actionable items. still not sure how to get exact specs out of the figma files. if an account is necessary or how to deal with things.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | inset.png | 3.35 MB | rkoller |
| #5 | Screenshot from 2022-07-28 18-59-02.png | 118.96 KB | omkar-pd |
| checkbox_states.png | 176.44 KB | rkoller | |
| checkbox_anatomy.png | 162.22 KB | rkoller | |
| pb_ct.png | 2.26 MB | rkoller |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
rkollerComment #5
omkar-pd commentedMade the changes as proposed resolution had to make some extra changes to adjust with the drupal design system. Please refer to the screenshot to check styling of unchecked, on hover, on focus, and checked states of checkboxes respectively.
Comment #6
omkar-pd commentedComment #7
rkollerthanks for working on the issue @omkar-pd! i've applied the patch and it looks good in safari, firefox and edge already. there are two small details.
1) for
input:checked:hoverthe change of color is missing.#232429forborder-colorandbackground-coloras well as thebox-shadowwithinset 0 0 0 1px #232429like it is already in place forinput:hover.2) the distance between the checkbox and the label currently is
10pxif i read it correctly? haven't added that in the issue summary but finally created an account on figma today and was able to take a closer look into the specs there. the distance between the checkbox and label is .5em (8px) in the figma file.but i guess someone more frontend savvy has also to take a look code wise since i am not a developer. i'll set the issue back to need work.
Comment #8
omkar-pd commented1) styled
input:checked:hover. No need to add box-shadow because if the checked checkbox is in focus then it will override that styling.2) changed margin-right to 8px.
Comment #9
rkollerthank you! applied the patches with the changes you've made and everything looks fine in all three aforementioned browsers i've tested. and you are right. the inset box-shadow on :checked:hover seems redundant and looks unnecessary since the background-color is already applying that color. But overridden it is not it has just no visual effect due to the redundant colors it is just one extra line of css code. I wonder if it would make sense to remove it from other places in core it is used (see the inset.png i've added for illustration - is for
/admin/modules) and open a follow up issue for that?and one general question. at the moment project_browser is a contrib module therefor the naming of selectors is not that much of an issue - in the merge request you only have element selectors and the class checkbox label. could that become a problem down the road as soon as project browser goes into core? meaning would it make sense to add class names instead of using element selectors? and isn't the checkbox-label a bit too vague and general and shouldn't it be at least
checkbox__label? i am not familiar with the exact CSS architecture conventions mandatory to core. so if the point about naming is valid then i would suggest a followup issue for that. and would set this issue to RTBC.Comment #10
omkar-pd commentedThe
box-shadow: inset 0 0 0 1px #232429;is not used:checked:hoverand used only on:hoverand it makes sense to use iton:hover. so I'm not sure from where we want to remove it from or maybe I'm not able to understand you're point here.and yes I should've used the class name before the element selector for ex:
.checkbox-label inputfor more specificity. and about naming even I'm not sure about the naming convention of the core or maybe we can use BEM naming convention (http://getbem.com/naming/).
Comment #11
tim.plunkettSorry, forgot that of course my gitlab review won't change the status :)
Comment #12
chrisfromredfinPostponing pending #3285889: [meta] Svelte app themability
Comment #13
tim.plunkettComment #14
chrisfromredfinComment #15
chrisfromredfinThis may be obsolete with #3318817: Improve the categories filter type in context to the rest of the filter component ui , unless we don't fix the checkboxes in that one.
Comment #16
narendrarHi @Chris, Can this issue be closed now?