uiowa / uids

UI Design System
http://uids.brand.uiowa.edu
7 stars 1 forks source link

4.x button updates #716

Closed GaryRidgway closed 2 years ago

GaryRidgway commented 2 years ago

Resolves #706 Resolves https://github.com/uiowa/uids/issues/710

To test

pyrello commented 2 years ago

@GaryRidgway It seems like the last thing you need to do here is write up testing instructions.

quamsta commented 2 years ago

I'll likely do a test run on adding this to the Brand Icon Browser today, not sure if that counts as a full review, but I'll put anything of note in here.

quamsta commented 2 years ago

@pyrello , @GaryRidgway does the building/compiling of the uids module's dist/ folder happen during a GitHub action? I attempted to import the components in the Brand Icon Browser, but I quickly realized that the statement I was using:

import { UidsBrandBar, UidsButton } from "uids";

...was still importing the old version of the UidsButton component. Does the dist/ folder need to be built in the 4.x-button-updates branch or will it happen automatically when it's merged into 4.x?

pyrello commented 2 years ago

@quamsta This process is not automated yet :|

Here is the relevant issue: https://github.com/uiowa/uids/issues/100. Running yarn build should take care of it. Sorry we forgot to do this the other day.

quamsta commented 2 years ago

@pyrello All good! I attempted to run yarn install and yarn build within Icon Browser's node_modules/uids folder, which did succeed, but upon trying to use the components, I received this error:

app.js:1201 Uncaught TypeError: Cannot read properties of null (reading 'isCE')
    at renderSlot (runtime-core.esm-bundler.js?9feb:2947:1)
    at Proxy._sfc_render (uids.es.js?175d:1:1)
    at renderComponentRoot (runtime-core.esm-bundler.js?d2dd:895:1)
    at ReactiveEffect.componentUpdateFn [as fn] (runtime-core.esm-bundler.js?d2dd:5059:1)
    at ReactiveEffect.run (reactivity.esm-bundler.js?89dc:185:1)
    at setupRenderEffect (runtime-core.esm-bundler.js?d2dd:5185:1)
    at mountComponent (runtime-core.esm-bundler.js?d2dd:4968:1)
    at processComponent (runtime-core.esm-bundler.js?d2dd:4926:1)
    at patch (runtime-core.esm-bundler.js?d2dd:4518:1)
    at mountChildren (runtime-core.esm-bundler.js?d2dd:4714:1)

Which I Googled around for and found this post from: https://github.com/vuejs/core/issues/4344

That's because you have two distinct copies of the Vue package being used, one in each package.

Vue uses a global singleton to track the current rendering instance - having more than one copy included will inevitably lead to such issues.

So I assumed I somehow duplicated or otherwise blew up my local UIDS components/package by attempting to run yarn build inside of node_modules, but I'm not sure.

bspeare commented 2 years ago

Changes that need to be documented

bspeare commented 2 years ago

Can we figure out a way to test these buttons with multiple background classes in this pr? I've been working with https://uids.brand.uiowa.edu/branches/feature_button_tertiary_fixes/components/preview/kitchen-sink.html to test, but it is not ideal.

bspeare commented 2 years ago

Should we add https://github.com/uiowa/uids/issues/714 to _border.scss here?

bspeare commented 2 years ago

Did we also want to remove the BEM nesting on https://github.com/uiowa/uids/blob/2a8460641aaf054fb23cdbe04e1d231de74f422c/src/components/button/button.scss?

pyrello commented 2 years ago

bttn--apply has been removed. It is only used in viewbooks and perhaps we should move it to https://github.com/uiowa/vuejs-site-template?

What is bttn--apply doing?

bspeare commented 2 years ago

I thought it was used in the viewbooks but I guess not. I would guess I added it for a uiowa.edu header button that is no longer in use.

GaryRidgway commented 2 years ago

Screen Shot 2022-06-30 at 10 08 47 AM it is used on viewbooks to add the proper icon to an apply button this is live on the admissions viewbook

GaryRidgway commented 2 years ago

it serves the same functionality as bttn--visit, bttn--ask, and bttn--connect

bspeare commented 2 years ago

@GaryRidgway I updated the class name in https://github.com/uiowa/uids/pull/716#issuecomment-1170505678. It is a different class than the viewbook example.

bspeare commented 2 years ago

I removed the BEM nesting and also removed the bttn--link modifier and added a entry for it in https://github.com/uiowa/uids/pull/716#issuecomment-1170505678.

GaryRidgway commented 2 years ago

Future work:

bspeare commented 2 years ago

Should we remove this serif option?

Screen Shot 2022-06-30 at 1 11 17 PM
pyrello commented 2 years ago

Should we remove this serif option?

I thought we specifically decided to add that. Do you want to not have it anymore?

bspeare commented 2 years ago

@pyrello I don't see us using the serif font any time soon. Maybe we could shelve it for now?