Icons, fonts, colors, oh my! Also, more Design System in Studio #2280

nucleogenesis · 2020-09-22T00:19:37Z

Description

Initially went to use the "Noto Sans" font throughout Studio and to apply font colors per Design System to Studio but along the way ended up making more changes than expected due to how Vuetify handles themes and colors and to avoid going through the entire app to apply styles to every component directly to make use of the proper KDS colors and text styles.

Vuetify theme now applies the Design System theme tokens over the previously used ones (overrides things like primary but keeps other apparently Studio-specific tokens)
Applies the proper text colors by using the theme-generated CSS variables. This means that all of the tokens defined in https://github.com/learningequality/kolibri-design-system/blob/v0.2.x/lib/styles/colorsDefault.js are available as CSS vars in Studio. For example, the text token can be used in CSS on Studio as --v-text-(base | lighten# | darken#) the same as other Vuetify theme colors are.
Implements KIconButton by placing it inside of the IconButton component (previously wrapped v-tooltip and v-btn together) and update some token keys uses previously targeting v-icon to use the Design System icon token names. This results in a different appearance of the tooltip for icon buttons by using the Design System tooltips.
Applied "Noto Sans" across the front end. In some cases, I kept the previous fonts as fall backs. Some places I left alone - like the Markdown Editor and similar areas where serif or symbol fonts were used.
Misc fixes to fix name-collisions between Vuetify and KDS token names and the styles applied with them by default in Vuetify (particularly the background-color on .link, .text classes.)
Adding KIconButton ended up being more work than I expected but by the time I realized it was going to be so much more I was kind of pot committed and finished it up. Tokens are renamed, Design System has a PR adding missing icons Adds add icon add_circle_outline kolibri-design-system#110, and I had to tweak some of the code to handle the fact that we're not putting a v-btn in the cards. This resulted in some additional logic around what happens when you click a channel/content/topic card or the buttons within it (copy, info, context, etc). There were resultant style issues that I fixed as well.
I also fixed an issue where the badge for a topic in the Clipboard (the purple thing saying how many children there are) was not centered. Just flagging this separately in case someone knows there is a Notion bug-bash issue (I'll give a look later on).

Issue Addressed (if applicable)

Fixes #2264

Steps to Test

Icons & Icon Buttons

Generally, it is a good idea to just play with every aspect of the app. Settings, channel edit, content edit, exercise editor,
Please go through the app everywhere that you know there is an Icon Button of any kind and please let me know if any are weirdly styled, improperly colored, or just don't do the thing they're supposed to do when clicked/hovered/etc.
Please verify that the info, copy, star and context menus for ChannelItem work in all cases (not just "My Channels" but also the public library channels (read only channels). Do this for items in the Clipboard as well. Also please do this on the Channel list and in ChannelEdit where content and topics are listed.
Test this logged in as well as not logged in ("Explore without an account") and ensure all of the buttons work and are where they ought to be.
Generally keep an eye out for any kind of weird text styles or color inconsistencies.
Do you see any fonts other than Noto Sans?

Implementation Notes (optional)

At a high level, how did you implement this?

Icon Buttons

I completely replaced the template for IconButton and made it a wrapper for KIconButton instead. I did this instead of replacing every implementation of IconButton in Studio with KIconButtons because I want to get thoughts from the team before spending the time changing that many files - also this will give us a chance to see if KIconButton can basically just be plugged in wherever we previously used the Studio-custom IconButton

The inspiration for this may be hard to explain but I'll try:

I overrode the Vuetify text color, which should apply also to icons.
There was no color prop for IconButton - despite my seeing it be passed somewhere - and even with it properly implemented, it was passed to v-btn which does not apply it to the icon properly.
I want to use the themeTokens from KDS for the colors now to avoid having to do it later, so I cannot dynamically override which styles are used depending on what prop value is passed
KIconButton already solves this basic problem by passing the color props to children directly to style them directly.

KIconButton - however - works exactly how we want it to:

Tooltip ✓
Color prop that is applied directly to the icon ✓
Does everything the v-btn did with events ✓

I add some styles that directly override the Vuetify styles

I'm not sure if this will have undesired consequences but I did not see any except for the one that inspired me to use KIconButton.

Does this introduce any tech-debt items?

Before merging this, I will go through and remove IconButton and replace every use of it with KIconButton. So - no once I do that.

codecov · 2020-09-22T00:38:48Z

Codecov Report

Merging #2280 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2280   +/-   ##
========================================
  Coverage    81.48%   81.48%           
========================================
  Files          293      293           
  Lines        14066    14066           
========================================
  Hits         11462    11462           
  Misses        2604     2604

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4395b2c...0647bf8. Read the comment docs.

indirectlylit · 2020-09-22T20:04:00Z

contentcuration/contentcuration/frontend/channelList/views/Channel/ChannelItem.vue

    :href="channelHref"
+    @click="goToChannelRoute"


this may cause a regression of #2156

The href is still there, but I e.preventDefault() the click. Do you know if that would still have the same a11y implications or does the existence of the href attribute suffice? I almost removed it but thought it might have a11y uses.

indirectlylit · 2020-09-22T20:18:10Z

contentcuration/contentcuration/templates/base.html

@@ -11,6 +11,8 @@
 <html dir="{{ LANGUAGE_BIDI|yesno:'rtl,ltr' }}" lang="{{ LANGUAGE_CODE }}">
    <head>
        {% block head %}
+        <link href="https://fonts.googleapis.com/css2?family=Noto+Sans:ital,wght@0,400;0,700;1,400;1,700&display=swap" rel="stylesheet">


In previous work, I specified all possibly subsets to support as wide a range of scripts as possible.

https://developers.google.com/fonts/docs/getting_started#specifying_script_subsets

Modern browsers support unicode-ranges, so the extra downloads will only affect older browsers.

Aside: it looks like we are loading italic font variants. As far as I know, no UI elements in the Kolibri Design System use italic, and Kolibri itself does not bundle italic variants. That said, I'm now realizing that this doesn't mean content doesn't use italics, and indeed our rich text exercise editors and HTML apps both support italics. Recommend we investigate and address this...

Eventually, it might be nice to add this to the static/fonts folder so that developers can also see what the UI will look like even if their internet gets disconnected

It would be much easier for devs to just install Noto to their local machine: https://www.google.com/get/noto/

Including font files locally requires LFS for large binary files, and some fairly complex CSS code to handle all the unicode ranges for different scripts like we do in Kolibri.

jayoshih

Code looks good! Just a few questions:

Should the title be bolded? I'm seeing a difference here
Do buttons need to be bolded?

I am running into the following error on the devserver, so I can't test more of the page right now on the demo server

Uncaught ReferenceError: regeneratorRuntime is not defined
    at channel_edit-651fde57276e1ec17b6b.js:1
    at channel_edit-651fde57276e1ec17b6b.js:1
    at Object.<anonymous> (channel_edit-651fde57276e1ec17b6b.js:1)
    at __w_pdfjs_require__ (channel_edit-651fde57276e1ec17b6b.js:1)
    at Object.<anonymous> (channel_edit-651fde57276e1ec17b6b.js:1)
    at __w_pdfjs_require__ (channel_edit-651fde57276e1ec17b6b.js:1)
    at Object.<anonymous> (channel_edit-651fde57276e1ec17b6b.js:1)
    at __w_pdfjs_require__ (channel_edit-651fde57276e1ec17b6b.js:1)
    at channel_edit-651fde57276e1ec17b6b.js:1
    at factory (channel_edit-651fde57276e1ec17b6b.js:1)

micahscopes · 2020-09-24T00:56:18Z

I finally got the channel edit app to load, and came across some errors:

vue.runtime.esm.js:619 [Vue warn]: Invalid prop: custom validator check failed for prop "icon".

found in

---> <KIcon> at node_modules/kolibri-design-system/lib/KIcon/index.vue
       <KButton> at node_modules/kolibri-design-system/lib/buttons-and-links/KButton.vue
         <KIconButton> at node_modules/kolibri-design-system/lib/buttons-and-links/KIconButton.vue
           <IconButton> at shared/views/IconButton.vue
             <VToolbar>
               <CurrentTopicView> at channelEdit/views/CurrentTopicView.vue
                 <VContent>
                   <TreeViewBase> at channelEdit/views/TreeView/TreeViewBase.vue
                     <TreeView> at channelEdit/views/TreeView/index.vue
                       <VApp>
                         <Root>

...

vue.runtime.esm.js:619 [Vue warn]: Error in render: "TypeError: Cannot read property 'icon' of undefined"

found in

---> <KIcon> at node_modules/kolibri-design-system/lib/KIcon/index.vue
       <KButton> at node_modules/kolibri-design-system/lib/buttons-and-links/KButton.vue
         <KIconButton> at node_modules/kolibri-design-system/lib/buttons-and-links/KIconButton.vue
           <IconButton> at shared/views/IconButton.vue
             <VToolbar>
               <CurrentTopicView> at channelEdit/views/CurrentTopicView.vue
                 <VContent>
                   <TreeViewBase> at channelEdit/views/TreeView/TreeViewBase.vue
                     <TreeView> at channelEdit/views/TreeView/index.vue
                       <VApp>
                         <Root>
...
vue.runtime.esm.js:1888 TypeError: Cannot read property 'icon' of undefined
    at VueComponent.svgIconComponent (index.vue:62)
    at Watcher.get (vue.runtime.esm.js:4479)
    at Watcher.evaluate (vue.runtime.esm.js:4584)
    at VueComponent.computedGetter [as svgIconComponent] (vue.runtime.esm.js:4836)
    at Object.get (vue.runtime.esm.js:2072)
    at Proxy.render (index.vue?7192:5)
    at VueComponent.Vue._render (vue.runtime.esm.js:3548)
    at VueComponent.updateComponent (vue.runtime.esm.js:4066)
    at Watcher.get (vue.runtime.esm.js:4479)
    at new Watcher (vue.runtime.esm.js:4468)

micahscopes · 2020-09-24T15:35:13Z

I was just checking out the exercise editor and noticed that the background color is green on selected v-cards:

micahscopes · 2020-09-24T15:40:09Z

I noticed that when hovering over topic tree list items on the channel edit page, their sizes change:

(I checked and verified the sizes are stable in develop)

top margin pushed the arrows to be flush with the bottom rather than centered vertically alongside the tab buttons

This commit is too big but it is not worth breaking down into smaller commits because much of it is interconnected and I should have done so in progress

the add circle outline icon is not in the design system at this time so we will need to address this later if desired.

nucleogenesis · 2020-09-25T00:00:30Z

Code looks good! Just a few questions:

Should the title be bolded? I'm seeing a difference here
Do buttons need to be bolded?

I am running into the following error on the devserver, so I can't test more of the page right now on the demo server

Uncaught ReferenceError: regeneratorRuntime is not defined
    at channel_edit-651fde57276e1ec17b6b.js:1
    at channel_edit-651fde57276e1ec17b6b.js:1
    at Object.<anonymous> (channel_edit-651fde57276e1ec17b6b.js:1)
    at __w_pdfjs_require__ (channel_edit-651fde57276e1ec17b6b.js:1)
    at Object.<anonymous> (channel_edit-651fde57276e1ec17b6b.js:1)
    at __w_pdfjs_require__ (channel_edit-651fde57276e1ec17b6b.js:1)
    at Object.<anonymous> (channel_edit-651fde57276e1ec17b6b.js:1)
    at __w_pdfjs_require__ (channel_edit-651fde57276e1ec17b6b.js:1)
    at channel_edit-651fde57276e1ec17b6b.js:1
    at factory (channel_edit-651fde57276e1ec17b6b.js:1)

I'm only seeing it bolded on the Channel Edit Details page - but that style appears to be set within the component using it.

The others aren't showing bolded for me in Chrome. Perhaps at bug bash we'll get some more reports?

KDS theme tokens contain `correct` field that is set to green (#43A047). Answers editor used this class to mark correct answers which introduced green background to them which should not be there.

to prevent height glitches on hover now when we use vertical options button instead of the horizontal one.

MisRob · 2020-09-25T10:48:27Z

@nucleogenesis @rtibbles I haven't managed to review everything. This is all I have for today:

I've just opened two pull requests:

Studio
nucleogenesis#1
Merged develop and added several fixes - please see commit messages

KDS
learningequality/kolibri-design-system#117
Fixes tooltips z-index issues:

Before	After

I noticed other minor issues but don't consider resolving them necessary to merge this PR before the bug hunt (it's just admin):

/administration/#/channels - download and pdf icons vertically misaligned

/administration/#/users - e-mail icon vertically misaligned

Review fixes for learningequality#2280

nucleogenesis requested review from jayoshih and micahscopes September 22, 2020 19:58

nucleogenesis added the needs review label Sep 22, 2020

nucleogenesis added this to the Vue Refactor milestone Sep 22, 2020

indirectlylit reviewed Sep 22, 2020

View reviewed changes

nucleogenesis marked this pull request as ready for review September 22, 2020 21:59

jayoshih added the qa-ready Create a demo server for this pull request label Sep 23, 2020

jayoshih reviewed Sep 23, 2020

View reviewed changes

nucleogenesis added 15 commits September 24, 2020 13:57

left & right icon buttons had unnecessary margin

471324b

top margin pushed the arrows to be flush with the bottom rather than centered vertically alongside the tab buttons

Use KIconButton, apply text styles and open sans

8818f7e

This commit is too big but it is not worth breaking down into smaller commits because much of it is interconnected and I should have done so in progress

wrong var name

6c16996

can click the card now

55e7823

margin-top on appbar in channel-edit icon buttons

1e11269

Use plus and minus icon keys in place of remove and add

db1dcbf

the add circle outline icon is not in the design system at this time so we will need to address this later if desired.

icon updates & point to branch on kds

32b7ea6

misc style fixes

c6dbd30

lint fixes

7a431e3

fix badge height in clipboard

18c6e55

test updates

0e5a0c2

proper kds link

af00182

different to href situation on channelitem

9f02d9f

Delete genstaged.py

abfcd26

properly un-excluded kds in webpack config excludes

7a7556a

nucleogenesis force-pushed the icons-colors-fonts branch from 340988f to 7a7556a Compare September 24, 2020 21:48

IconButtons on channel edit at top router-linked

2f97767

Michaela Robosova added 4 commits September 25, 2020 11:13

Merge branch 'develop' into kds-fixes

27d348b

Fix conflicting correct class

60e3286

KDS theme tokens contain `correct` field that is set to green (#43A047). Answers editor used this class to mark correct answers which introduced green background to them which should not be there.

Increase list item height

bfc515e

to prevent height glitches on hover now when we use vertical options button instead of the horizontal one.

Center a badge vertically

db694ff

This was referenced Sep 25, 2020

Review fixes for #2280 #2301

Closed

Review fixes for #2280 nucleogenesis/studio#1

Merged

KIconButton - Use "zIndex" property instead of "style" in UiTooltip learningequality/kolibri-design-system#117

Merged

Merge pull request #1 from MisRob/kds-fixes

0647bf8

Review fixes for learningequality#2280

rtibbles merged commit e78b0b8 into learningequality:develop Sep 25, 2020

MisRob mentioned this pull request Oct 2, 2020

Tooltips appear behind some elements #2342

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Icons, fonts, colors, oh my! Also, more Design System in Studio #2280

Icons, fonts, colors, oh my! Also, more Design System in Studio #2280

nucleogenesis commented Sep 22, 2020 •

edited

Loading

codecov bot commented Sep 22, 2020 •

edited

Loading

indirectlylit Sep 22, 2020

nucleogenesis Sep 22, 2020

indirectlylit Sep 22, 2020

jayoshih Sep 23, 2020

indirectlylit Sep 23, 2020

jayoshih left a comment

micahscopes commented Sep 24, 2020

micahscopes commented Sep 24, 2020

micahscopes commented Sep 24, 2020

nucleogenesis commented Sep 25, 2020

MisRob commented Sep 25, 2020

Icons, fonts, colors, oh my! Also, more Design System in Studio #2280

Icons, fonts, colors, oh my! Also, more Design System in Studio #2280

Conversation

nucleogenesis commented Sep 22, 2020 • edited Loading

Description

Issue Addressed (if applicable)

Steps to Test

Implementation Notes (optional)

At a high level, how did you implement this?

Does this introduce any tech-debt items?

codecov bot commented Sep 22, 2020 • edited Loading

Codecov Report

indirectlylit Sep 22, 2020

Choose a reason for hiding this comment

nucleogenesis Sep 22, 2020

Choose a reason for hiding this comment

indirectlylit Sep 22, 2020

Choose a reason for hiding this comment

jayoshih Sep 23, 2020

Choose a reason for hiding this comment

indirectlylit Sep 23, 2020

Choose a reason for hiding this comment

jayoshih left a comment

Choose a reason for hiding this comment

micahscopes commented Sep 24, 2020

micahscopes commented Sep 24, 2020

micahscopes commented Sep 24, 2020

nucleogenesis commented Sep 25, 2020

MisRob commented Sep 25, 2020

nucleogenesis commented Sep 22, 2020 •

edited

Loading

codecov bot commented Sep 22, 2020 •

edited

Loading