Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sage: Integrate Pine into icons #1890

Merged
merged 85 commits into from
Jun 6, 2024
Merged

Conversation

monicawheeler
Copy link
Collaborator

@monicawheeler monicawheeler commented May 17, 2024

Description

Updates components and documentation to use pds-icon. Switching over the new icon system should help to eliminate the current manual process.

Screenshots

Minimal visual changes should be expected.

Testing in sage-lib

  • Navigate to each component for Rails & React
  • Verify icons render as expected.
  • Verify icons render in docs as expected

Testing in kajabi-products

  1. (BREAKING) Converts icon font to pds-icons in components and documentation.
    • A full regression of the kajabi admin should be performed to make sure all icons still display as expected.

Related

DSS-623

@monicawheeler monicawheeler added the Do not merge Requirements must be met before merging label May 17, 2024
@monicawheeler monicawheeler self-assigned this May 17, 2024
Copy link
Contributor

github-actions bot commented May 17, 2024

Dependency Review

The following issues were found:
  • ✅ 0 vulnerable package(s)
  • ✅ 0 package(s) with incompatible licenses
  • ✅ 0 package(s) with invalid SPDX license definitions
  • ⚠️ 1 package(s) with unknown licenses.
See the Details below.

License Issues

yarn.lock

PackageVersionLicenseIssue Type
@types/node16.18.98NullUnknown License
Denied Licenses: LGPL-2.0, AAL, Adobe-2006, Afmparse, Artistic-1.0, Artistic-1.0-cl8, Artistic-1.0-Perl, Beerware, blessing, Borceux, CECILL-B, ClArtistic, Condor-1.1, Crossword, CrystalStacker, diffmark, DOC, EFL-1.0, EFL-2.0, Fair, FSFUL, FSFULLR, Giftware, HPND, IJG, Leptonica, LPL-1.0, LPL-1.02, MirOS, mpich2, NASA-1.3, NBPL-1.0, Newsletr, NLPL, NRL, OGTSL, OLDAP-1.1, OLDAP-1.2, OLDAP-1.3, OLDAP-1.4, psutils, Qhull, Rdisc, RSA-MD, Spencer-86, Spencer-94, TU-Berlin-1.0, TU-Berlin-2.0, Vim, W3C-19980720, W3C-20150513, Wsuipa, WTFPL, xinetd, Zed, Zend-2.0, ZPL-1.1, AGPL-1.0-only, AGPL-1.0-or-later, AGPL-3.0-only, AGPL-3.0-or-later, APSL-1.0, APSL-1.1, APSL-1.2, APSL-2.0, CPAL-1.0, EUPL-1.0, EUPL-1.1, EUPL-1.2, NPOSL-3.0, OSL-1.0, OSL-1.1, OSL-2.0, OSL-2.1, OSL-3.0, RPSL-1.0, SSPL-1.0, CAL-1.0, CAL-1.0-Combined-Work-Exception, Parity-6.0.0, Parity-7.0.0, RPL-1.1, RPL-1.5, EPL-1.0, EPL-2.0, ErlPL-1.1, IPL-1.0, LGPL-2.0-only, LGPL-2.0-or-later, LGPL-2.1-only, LGPL-2.1-or-later, LGPL-2.1, LGPL-3.0-only, LGPL-3.0-or-later, LGPL-3.0, MPL-1.0, MPL-1.1, MPL-2.0, MPL-2.0-no-copyleft-exception, MS-RL, SPL-1.0, BSD-Protection, copyleft-next-0.3.0, copyleft-next-0.3.1, GPL-1.0-only, GPL-1.0-or-later, GPL-1.0, GPL-2.0-only, GPL-2.0-or-later, GPL-2.0, GPL-3.0-only, GPL-3.0-or-later, GPL-3.0, QPL-1.0, Sleepycat

Scanned Manifest Files

package.json
  • @pine-ds/icons@^7.0.1
yarn.lock

package.json Outdated Show resolved Hide resolved
@pixelflips pixelflips force-pushed the feature/integrate-pine-icons branch from b94baf6 to a08ca7b Compare May 22, 2024 00:22
@ju-Skinner ju-Skinner force-pushed the feature/integrate-pine-icons branch 2 times, most recently from 1a4f690 to 5b7c438 Compare May 22, 2024 02:37
Copy link
Contributor

@anechol anechol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Icons are coming in fine in Rails, but I can't see them on the React side. Additionally, because of them being web components, there is now a fraction of a second on page load with no cache where the icon isn't showing up. I can't imagine there will be complaints about it, but just making a note of it here, since this is being updated app-wide.

@pixelflips
Copy link
Member

Icons are coming in fine in Rails, but I can't see them on the React side. Additionally, because of them being web components, there is now a fraction of a second on page load with no cache where the icon isn't showing up. I can't imagine there will be complaints about it, but just making a note of it here, since this is being updated app-wide.

You will need to be running your local pine for the react side. Also, make sure the ports match your local instance in the packages/sage-react/.storybook/preview-head.html file.

@QuintonJason QuintonJason self-requested a review May 30, 2024 13:36
Copy link
Contributor

@QuintonJason QuintonJason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-05-30 at 8 34 13 AM

In the buttons, the icons are showing twice, once as the ::before and another as the pds-icon

@QuintonJason QuintonJason self-requested a review May 30, 2024 14:51
Copy link
Contributor

@QuintonJason QuintonJason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After changing line 86 of packages/sage-react/package.json to "@kajabi/sage-assets": "*", and restarted the server, everything was good to go

@monicawheeler
Copy link
Collaborator Author

monicawheeler commented May 30, 2024

@pixelflips - the Category item page has an alert, http://localhost:4000/pages/component/catalog_item?tab=preview, is missing the icon

Local Live
Screenshot 2024-05-30 at 2 15 41 PM Screenshot 2024-05-30 at 2 15 45 PM

Others like this:

@monicawheeler
Copy link
Collaborator Author

Property is missing the icons
http://localhost:4000/pages/component/property?tab=preview
Screenshot 2024-05-30 at 2 21 20 PM

@monicawheeler
Copy link
Collaborator Author

Typeahead is missing the icons in the dropdown
Screenshot 2024-05-30 at 2 22 54 PM

@pixelflips
Copy link
Member

@monicawheeler those should all be addressed now, docs just had incorrect icon naming.

alert icons - done
property - done / forgot to push the commit for this one
typeahead - done

@monicawheeler
Copy link
Collaborator Author

Screenshot 2024-05-30 at 2 38 27 PM
On local, the arrows are not aligned properly in react

@ju-Skinner ju-Skinner force-pushed the feature/integrate-pine-icons branch from fdb306f to 6ded28c Compare June 6, 2024 15:48
@ju-Skinner ju-Skinner force-pushed the feature/integrate-pine-icons branch from 6ded28c to 4ed3004 Compare June 6, 2024 16:49
@pixelflips pixelflips merged commit 74c7d4e into develop Jun 6, 2024
7 checks passed
@pixelflips pixelflips deleted the feature/integrate-pine-icons branch June 6, 2024 19:51
@pixelflips pixelflips mentioned this pull request Jun 6, 2024
1 task
ju-Skinner pushed a commit that referenced this pull request Jun 7, 2024
* fix: removes dasherize from variable used for attributes (#1893)

* chore: update browserslist dependency

* fix: removes dasherize variable used for attributes

* fix: remove update

* fix: adjust id conditional

* Sage: Integrate Pine into icons (#1890)

* chore: update browserslist dependency

* feat: add pine icons to sage

* feat(icon): work pine-icon into sage-icon

* feat(icon): add pds-icon to both rails and react components

* feat: convert rails button icons to pine

* fix: correct icon z-index

* revert: scripts in manager-head

* feat: convert rails icon to pine

* fix: add expandable card styles for pds-icon

* feat(alert): convert rails icon to pine

* fix(avatar): add styles for pds-icon

* feat(badge): convert rails icon to pine

* feat(banner): convert rails icon to pine

* feat(breadcrumbs): convert rails icon to pine

* feat(choice): convert rails icon to pine

* feat(copytext): convert rails icon to pine

* fix(button): add icon for disclosure btn

* fix(expandablecard): adjust icon spacing

* feat(iconcard): convert rails icon to pine

* fix: correct missing icons

* feat(forminput): convert rails icon to pine

* fix(formsection): correct missing icons

* feat(formselect): convert rails icon to pine

* fix(formselect): add message icon

* feat(textarea): convert rails icon to pine

* fix(hint): correct icon alignment

* feat(link): convert rails icon to pine

* fix: linting errors

* ci: add step to configure authToken for NPM

* test(react-icon): comment out aria-lable test

* feat(navlink): convert rails icon to pine

* feat(pageheading): convert help link icon to pine

* docs(icon): convert icons to pine

* feat(dropdownitem): convert rails icon to pine

* feat(search): convert rails icon to pine

* feat(statusicon): convert rails icon to pine

* feat(sortable): convert rails icon to pine

* feat(label): convert rails icon to pine

* feat(tab): convert rails icon to pine

* fix(tabs): correct icon name typo

* fix(search): correct toolbar alignment

* fix(catalogitem): fix missing icon

* fix: various icon bug fixes

* fix(docs): correct home icon

* fix(pagination): removes sage icon dependency

* feat(toast): convert rails icon to pine

* fix: correct linting errors

* fix: remove commented code

* fix(checkbox): convert check icon to svg

* fix(hero): convert play icon to svg

* fix(select): update icon-base to include pine

* fix: update icon-base to include pine

* fix: removes pine-ds/icons as dependency

* fix(button): convert classes to tokens

* feat(button-react): convert icons to pine

* feat(badge-react): convert icons to pine

* fix(button-react): convert icon classes to function

* fix: correct linter error

* fix(choice): update alignments and convert icons

* feat(copytext-react): convert icons to pine

* feat(link-react): convert icons to pine

* feat(link-react): convert icons to pine

* style: correct missing semicolon

* docs(panel): add icons to preview

* fix(badge): change to self closing

* feat(toast-react): convert icons to pine

* feat(search): convert icons to pine

* docs(search): add disabled search preview

* fix(icon): adjust text alignment variant

* fix: correct linting errors

* fix(iconcard): fix sizing and styling issue

* fix: search styling adjustments

* fix(tag): styling fixes

* fix(button): correct type

* style: lint fixes

* fix: rename icon

* fix: correct style syntax

* docs: provide additional info in preview-head

* feat(property): convert icons to pine

* docs: correct icon names

* docs: remove legacy instructions

* fix: update pds-icon version

* feat(banner): add pine icons

* fix: correct linting errors

* chore: update pine-ds/icons to v7.0.1

---------

Co-authored-by: Julian Skinner <[email protected]>
Co-authored-by: Phillip Lovelace <[email protected]>

---------

Co-authored-by: Monica Wheeler <[email protected]>
Co-authored-by: Julian Skinner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants