Skip to content

Conversation

@bennypowers
Copy link
Member

@bennypowers bennypowers commented May 23, 2023

What I did

  1. remove the existing icons build script
  2. replace it with a script that copies over patternfly icons (mostly fontawesome 5)

Testing Instructions

  1. DP
  2. copy locally into RHDS and check footer, navigation-secondary

Notes to Reviewers

  1. pretty sure this is a breaking change. I think we can avoid a major release by restoring the old build script and running the new one on top of it, then any icons of the same file name can be considered 'fixes' without breaking the path they're stored at breakage has been avoided

@changeset-bot
Copy link

changeset-bot bot commented May 23, 2023

🦋 Changeset detected

Latest commit: 8346fd1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@patternfly/elements Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added work in progress POC / Not ready for review tools Development and build tools AT passed Automated testing has passed labels May 23, 2023
@github-actions github-actions bot added the functionality Functionality, typically pertaining to the JavaScript. label May 28, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 28, 2023

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit 37a1d96
😎 Deploy Preview https://deploy-preview-2502--patternfly-elements.netlify.app/

To edit notification comments on pull requests, go to your Netlify site settings.

@bennypowers bennypowers marked this pull request as ready for review May 28, 2023 09:24
@github-actions github-actions bot added demo Updating demo pages doc labels May 28, 2023
@bennypowers bennypowers enabled auto-merge (squash) May 28, 2023 09:51
@heyMP heyMP self-requested a review June 5, 2023 13:53
@zeroedin
Copy link
Contributor

zeroedin commented Jun 6, 2023

Should these generated files be in the.gitignore

Screenshot 2023-06-06 at 10 00 15 AM

@bennypowers
Copy link
Member Author

@zeroedin I can't reproduce that - i tried busting the wireit cache, git clean, npm ci, and npx ts-node --esm ./scripts/icons.ts, and everything copies into elements/pf-icon/icons

@zeroedin
Copy link
Contributor

zeroedin commented Jun 6, 2023

@zeroedin I can't reproduce that - i tried busting the wireit cache, git clean, npm ci, and npx ts-node --esm ./scripts/icons.ts, and everything copies into elements/pf-icon/icons

I'm running:

git clean -fdx
nvm use
npm ci
npm run start

@bennypowers
Copy link
Member Author

@zeroedin I ran that and it works as expected here... i'll try cloning into /tmp

@bennypowers
Copy link
Member Author

I cloned into /tmp/ and ran the build, and everything came out as expected. fedora 38

@heyMP
Copy link
Contributor

heyMP commented Jun 6, 2023

I cloned into /tmp/ and ran the build, and everything came out as expected. fedora 38

I'll try with a fresh copy

@zeroedin
Copy link
Contributor

zeroedin commented Jun 6, 2023

I cloned into /tmp/ and ran the build, and everything came out as expected. fedora 38

When I do the same here is my output:

https://gist.github.com/zeroedin/fe15286ee245e0ba92318ea9012b34fa

@bennypowers
Copy link
Member Author

bennypowers commented Jun 6, 2023

I was able to reproduce on macos. ok i think this is a POSIX/cp -R issue on macos

@zeroedin
Copy link
Contributor

zeroedin commented Jun 6, 2023

I was able to reproduce on macos. ok i think this is a POSIX/cp -R issue on macos

This maybe related:

https://www.autodesk.com/support/technical/article/caas/sfdcarticles/sfdcarticles/The-cp-R-command-works-differently-on-Mac-OS-X-than-on-Linux.html

@heyMP
Copy link
Contributor

heyMP commented Jun 6, 2023

I was able to reproduce on macos. ok i think this is a POSIX/cp -R issue on macos

or maybe the trailing slashes on the directories? Trialing slash is saying "take all of the contents of this directory"

@bennypowers
Copy link
Member Author

yeah. https://unix.stackexchange.com/questions/18712/difference-between-cp-r-and-cp-r-copy-command

@bennypowers
Copy link
Member Author

i'll refactor to use node fs apis

@heyMP
Copy link
Contributor

heyMP commented Jun 6, 2023

@bennypowers
Copy link
Member Author

Try again, this should be portable to cupertinux now.

patternfly-elements on  fix/patternflyicon [$] via  v18.12.1 took 36s 
❯ uname -r                                                                                                    (base) 
6.3.4-201.fc38.x86_64

patternfly-elements on  fix/patternflyicon [$] via  v18.12.1 
❯ ls elements/pf-icon/icons                                                                                   (base) 
fab/  far/  fas/  patternfly/

patternfly-elements on  fix/patternflyicon [$] via  v18.12.1 
❯ ls elements/pf-icon/                                                                                        (base) 
BaseIcon.css  BaseIcon.ts  demo/  docs/  icons/  pf-icon.css  pf-icon.ts  README.md  test/

patternfly-elements on  fix/patternflyicon [$] via  v18.12.1 
❯ git log --name-status HEAD^..HEAD                                                                           (base) 
commit 8346fd193dc0c212b4689bce15b9386f40ccf3f9 (HEAD -> fix/patternflyicon, origin/fix/patternflyicon)
Author: Benny Powers <web@bennypowers.com>
Date:   Tue Jun 6 19:01:04 2023 +0300

    chore: mac-friendly icon build

M       scripts/icons.ts

@zeroedin
Copy link
Contributor

zeroedin commented Jun 6, 2023

Try again, this should be portable to cupertinux now.

Icon location look good to me now and no extra "added" file changes after build.

@zeroedin
Copy link
Contributor

zeroedin commented Jun 6, 2023

Ran a build and copied icons files and js over to RHDS, things look ok downstream from what I can tell as well.

Copy link
Contributor

@zeroedin zeroedin left a comment

Choose a reason for hiding this comment

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

Letters, Glyphs, Trademarks, Monograms

@heyMP
Copy link
Contributor

heyMP commented Jun 6, 2023

Fixed for me! Let's go! 🚀

Copy link
Contributor

@heyMP heyMP left a comment

Choose a reason for hiding this comment

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

Let's go 🚀

@bennypowers bennypowers merged commit 12c59e9 into main Jun 6, 2023
@bennypowers bennypowers deleted the fix/patternflyicon branch June 6, 2023 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AT passed Automated testing has passed demo Updating demo pages functionality Functionality, typically pertaining to the JavaScript. tools Development and build tools work in progress POC / Not ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants