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

fix(tools): dev server icon and colours #2733

Merged
merged 7 commits into from
Apr 11, 2024

Conversation

bennypowers
Copy link
Member

What I did

  1. change dev server header colour to black
  2. use correct url for dev server header icon

@bennypowers bennypowers requested a review from zeroedin March 31, 2024 09:02
Copy link

changeset-bot bot commented Mar 31, 2024

🦋 Changeset detected

Latest commit: 0cf74b6

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

This PR includes changesets to release 1 package
Name Type
@patternfly/pfe-tools Patch

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

@bennypowers bennypowers enabled auto-merge (squash) March 31, 2024 09:02
@github-actions github-actions bot added the tools Development and build tools label Mar 31, 2024
Copy link

netlify bot commented Mar 31, 2024

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit f17f672
😎 Deploy Preview https://deploy-preview-2733--patternfly-elements.netlify.app/

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

@github-actions github-actions bot added the AT passed Automated testing has passed label Mar 31, 2024
@bennypowers bennypowers force-pushed the fix/tools/dev-server-icon-colours branch from 8cc602f to 5f1f462 Compare March 31, 2024 12:22
Copy link
Collaborator

@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.

Having issues running dev server on this branch:

Now using node v20.10.0 (npm v10.2.3)
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR! 
npm ERR! While resolving: @patternfly/[email protected]
npm ERR! Found: [email protected]
npm ERR! node_modules/esbuild
npm ERR!   esbuild@"^0.20.2" from @patternfly/[email protected]
npm ERR!   tools/pfe-tools
npm ERR!     @patternfly/[email protected]
npm ERR!     node_modules/@patternfly/pfe-tools
npm ERR!       workspace tools/pfe-tools from the root project
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer esbuild@"^0.16.17 || ^0.17.16 || ^0.18.1 || ^0.19.5" from [email protected]
npm ERR! node_modules/esbuild-plugin-lit-css
npm ERR!   esbuild-plugin-lit-css@"^2.1.0" from @patternfly/[email protected]
npm ERR!   tools/pfe-tools
npm ERR!     @patternfly/[email protected]
npm ERR!     node_modules/@patternfly/pfe-tools
npm ERR!       workspace tools/pfe-tools from the root project
npm ERR! 
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.

If I upgrade the following deps in tools/pfe-tools/package.json to:

    "esbuild-plugin-lit-css": "^3.0.0",
    "esbuild-plugin-minify-html-literals": "^2.0.0",

And remove the following comment @ts-expect-error

❌ [build:tools] exited with exit code 1. Output:

tools/pfe-tools/dev-server/config.ts:19:1 - error TS2578: Unused '@ts-expect-error' directive.

19 // @ts-expect-error: looks like this was broken upstream. remove this expect-error directive when they fix it
   

things work. If this matches expectations let me know I'll push this as a commit.

@bennypowers
Copy link
Member Author

@zeroedin please do

Copy link
Collaborator

@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.

Logo good, to merge ➡️

@zeroedin
Copy link
Collaborator

zeroedin commented Apr 10, 2024

unexpected test failures. taking a look.

Update:

@bennypowers I pushed a fix for the failing pf-card test PTAL, working with Nikki to fix the pf-chip-group failing test. element.focus() SHOULD focus the first chip but doesn't seem to do that in this test.

Doing the following:

const firstChip = element.querySelector('pf-chip')!;
firstChip.focus();

works, but I don't think we should have to do this, the first chip should become focusable via RTIC IIRC and that's where the focus should land given delegatesFocus?

Either way if you feel like poking at it please do otherwise will return back to it in the morning.

@github-actions github-actions bot added the tests Related to testing label Apr 10, 2024
@github-actions github-actions bot added functionality Functionality, typically pertaining to the JavaScript. and removed tests Related to testing labels Apr 11, 2024
@bennypowers bennypowers merged commit d50a651 into main Apr 11, 2024
12 of 13 checks passed
@bennypowers bennypowers deleted the fix/tools/dev-server-icon-colours branch April 11, 2024 05: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 functionality Functionality, typically pertaining to the JavaScript. ready to merge tools Development and build tools
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants