FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: Move simple slot-only examples into JS by castastrophe · Pull Request #1730 · patternfly/patternfly-elements · GitHub

/ patternfly-elements Public

feat: Move simple slot-only examples into JS - #1730

Closed
castastrophe wants to merge 20 commits into
masterfrom
feat-compile-sass-html-separately
ConversationCommits (20)ChecksFiles changed
Closed

feat: Move simple slot-only examples into JS #1730
castastrophe wants to merge 20 commits into
masterfrom
feat-compile-sass-html-separately

Conversation

castastrophe commented Jul 27, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Separate out styles from HTML in template creation

This lets us put very simple templates in the JS file instead of having them in a separate html asset but we can maintain the separate Sass file for styles.

Testing instructions

Check for regressions in:

  • pfe-accordion
  • pfe-codeblock
  • pfe-collapse
  • pfe-markdown
  • pfe-navigation
  • pfe-progress-indicator
  • pfe-select

Ready-for-merge Checklist

  • Expected files: all files in this pull request are related to one request or issue (no stragglers or scope-creep).
  • Repository compiles and tests pass.
  • Changelog updated (required for fix and feat changes).
  • Documentation updated or added, if necessary.

Merging

Please squash when merging and ensure your commit message uses conventional commit formatting.

Be sure to share your updates with the patternfly-elements-contribute@redhat.com mailing list!

castastrophe added tools Development and build tools ready: branch testing Test the component from a user-perspective. Try to break it! ready: code review Ready for code review! priority: low Severity level: 3 labels Jul 27, 2021
github-actions Bot added the functionality Functionality, typically pertaining to the JavaScript. label Jul 27, 2021

netlify Bot commented Jul 27, 2021 •
edited
Loading

Copy link
Copy Markdown

✔️ Deploy Preview for patternfly-elements ready!

🔨 Explore the source changes: 1f05e54

🔍 Inspect the deploy log: https://app.netlify.com/sites/patternfly-elements/deploys/6103f06df00f840007635d72

😎 Browse the preview: https://deploy-preview-1730--patternfly-elements.netlify.app

github-actions Bot added the docs Documentation updates label Jul 27, 2021
castastrophe removed the ready: branch testing Test the component from a user-perspective. Try to break it! label Jul 27, 2021
castastrophe requested a review from kylebuch8 July 27, 2021 20:08
castastrophe added priority: medium Severity level: 2 run e2e Trigger automated visual regression tests size: sm Sizing label; indicates a quick, relatively easy task and removed priority: low Severity level: 3 labels Jul 27, 2021
castastrophe added a commit that referenced this pull request Jul 27, 2021
feat: Pull out styles/html into separate PR #1730
45ed990
castastrophe changed the title feat: Move simple slot-only examples into JS feat: Move simple slot-only examples into JS Jul 28, 2021
github-actions Bot added the AT passed Automated testing has passed label Jul 28, 2021
castastrophe enabled auto-merge (squash) July 28, 2021 04:11
castastrophe added a commit that referenced this pull request Jul 28, 2021
feat: gracefully fail on duplicate registration (#1728)
d3ea7fa 
* feat: Pull out polyfill and pfelement updates

* feat: Update changelog

* feat: Bring in update to parsing tool

* feat: Polyfills

* feat: Add comment for polyfill file

* feat: Update polyfill and todos

* feat: Pull out styles/html into separate PR #1730

* feat: Update todos and polyfills listing

* feat: Update test cases

* feat: Split out tests into 2

* feat: Pull out context tests into separate file

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
castastrophe changed the title feat: Move simple slot-only examples into JS feat: Move simple slot-only examples into JS Jul 28, 2021
castastrophe changed the title feat: Move simple slot-only examples into JS feat: Move simple slot-only examples into JS Jul 28, 2021
kylebuch8 requested changes Jul 28, 2021
View reviewed changes
Comment thread elements/pfelement/README.md
return "my-element.scss";
}

// Alternative: Embed the styles directly in the JS.

kylebuch8 Jul 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@castastrophe Can we get rid of this alternative? I'd really like to continue with using just scss files instead of suggesting this alternative.

castastrophe Jul 28, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm not sure it necessarily hurts to make both available. One example is a component with only styles for :host([hidden]) { display: none; }. Does that need Sass and a separate file at that point? Maybe not yet.

castastrophe Jul 28, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

These are the styles in the pfe-number.scss file:

:host {
  display: inline;
  white-space: nowrap;
}

castastrophe Jul 28, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

One more example from pfe-datetime.scss:

:host {
    display: inline;
}

castastrophe Jul 28, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I just thought of something else cool you could use styles for:

get styles() {
   return `<link rel="stylesheet" href="https://mycdn/external-styles.css">`;
}

Nothing forcing you to use the style tag per say in this approach.

Comment thread docs/docs/develop/html.md
castastrophe requested a review from kylebuch8 July 29, 2021 23:57
auto-merge was automatically disabled October 13, 2021 15:16

Pull request was closed

bennypowers deleted the feat-compile-sass-html-separately branch June 25, 2024 07:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

kylebuch8 Awaiting requested review from kylebuch8

Assignees

No one assigned

Labels

AT passed Automated testing has passed docs Documentation updates functionality Functionality, typically pertaining to the JavaScript. priority: medium Severity level: 2 ready: code review Ready for code review! run e2e Trigger automated visual regression tests size: sm Sizing label; indicates a quick, relatively easy task tools Development and build tools

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL