Skip to content

fix: Reduce unnecessary interpolation and function calls #1042

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.

Sign up for GitHub

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

Jump to bottom
Merged
kylebuch8 merged 41 commits into from
Dec 2, 2020
Merged

fix: Reduce unnecessary interpolation and function calls #1042

kylebuch8 merged 41 commits into from
Dec 2, 2020

Conversation

@castastrophe
Copy link
Contributor

@castastrophe castastrophe commented Aug 13, 2020 •
edited
Loading

Goal 1: Reduce complexity from the sass files.

Went through the styles and tried to reduce complexity by removing unnecessary interpolation and opting for verbosity rather than mixins (i.e., pfe-print-local) so that sass is easier to parse.

Goal 2: Tidy up and reduce complexity in pfe-band layouts.

Preview

Link(s) to demo page(s) where this element can be viewed:

Testing instructions

  1. Needs regression testing to ensure no changes.

Browser requirements

Your component should work in all of the following environments:

  • Latest 2 versions of Edge
  • Internet Explorer 11 (should be useable, not pixel perfect)
  • Latest 2 versions of Firefox (one on Mac OS, one of Windows OS)
  • Firefox 68 (or latest version for Red Hat Enterprise Linux distribution)
  • Latest 2 versions of Chrome (one on Mac OS, one of Windows OS)
  • Latest 2 versions of Safari
  • Android mobile device (such as the Galaxy S9)
  • Apple mobile device (such as the iPhone X)
  • Apple tablet device (such as the iPhone Pro)

Ready-for-merge Checklist

Check off items as they are completed. Feel free to delete items if they are not applicable.

  • Expected files: all files in this pull request are related to one request or issue (no stragglers or scope-creep).
  • Browser testing passed.
  • Repository compiles and tests pass.
  • Changelog updated (not needed for documentation updates).

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 castastrophe added feature New feature or request ready: branch testing Test the component from a user-perspective. Try to break it! priority: low Severity level: 3 needs changelog Be sure to update the Changelog before merging. styles An issue or PR pertaining only to CSS/Sass labels Aug 13, 2020
@castastrophe castastrophe marked this pull request as ready for review August 13, 2020 15:46
@castastrophe castastrophe added ready: browser testing Test the component in the supported browser environments. ready: code review Ready for code review! labels Aug 13, 2020
@castastrophe castastrophe added run e2e Trigger automated visual regression tests and removed needs changelog Be sure to update the Changelog before merging. labels Sep 11, 2020
@castastrophe castastrophe removed the run e2e Trigger automated visual regression tests label Oct 8, 2020
@castastrophe castastrophe added the on hold wait to merge label Oct 20, 2020
@castastrophe
Copy link
Contributor Author

castastrophe commented Oct 20, 2020

On hold pending the base class updates

@castastrophe castastrophe added this to the 1.0 release milestone Nov 17, 2020
@castastrophe castastrophe removed the on hold wait to merge label Nov 17, 2020
@castastrophe castastrophe removed ready: branch testing Test the component from a user-perspective. Try to break it! ready: browser testing Test the component in the supported browser environments. labels Nov 25, 2020
@castastrophe castastrophe added next release PRs that need to merge before the next release goes out size: md Sizing label; indicates a moderate difficulty level or amount of work and removed feature New feature or request labels Nov 30, 2020
starryeyez024
starryeyez024 requested changes Nov 30, 2020
View reviewed changes
@github-actions github-actions bot added demo Updating demo pages docs Documentation updates functionality Functionality, typically pertaining to the JavaScript. tests Related to testing labels Nov 30, 2020
starryeyez024
starryeyez024 requested changes Dec 1, 2020
View reviewed changes
starryeyez024
starryeyez024 reviewed Dec 1, 2020
View reviewed changes
starryeyez024
starryeyez024 reviewed Dec 1, 2020
View reviewed changes
starryeyez024
starryeyez024 previously requested changes Dec 2, 2020
View reviewed changes
Copy link
Member

@starryeyez024 starryeyez024 left a comment

Choose a reason for hiding this comment

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

LGTM

@castastrophe castastrophe added ready to merge and removed ready: code review Ready for code review! labels Dec 2, 2020
kylebuch8
kylebuch8 approved these changes Dec 2, 2020
View reviewed changes
Copy link
Contributor

@kylebuch8 kylebuch8 left a comment

Choose a reason for hiding this comment

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

Lets Get This Merged!

@kylebuch8 kylebuch8 dismissed starryeyez024’s stale review December 2, 2020 20:38

You should've hit approve.

@kylebuch8 kylebuch8 merged commit f4880d6 into master Dec 2, 2020
@kylebuch8 kylebuch8 deleted the fix-simplify-sass branch December 2, 2020 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kylebuch8 kylebuch8 kylebuch8 approved these changes

@starryeyez024 starryeyez024 starryeyez024 left review comments

Assignees

No one assigned

Labels

demo Updating demo pages docs Documentation updates functionality Functionality, typically pertaining to the JavaScript. next release PRs that need to merge before the next release goes out priority: low Severity level: 3 ready to merge size: md Sizing label; indicates a moderate difficulty level or amount of work styles An issue or PR pertaining only to CSS/Sass tests Related to testing

Projects

None yet

Milestone

1.0 release

Development

Successfully merging this pull request may close these issues.

4 participants

@castastrophe @kylebuch8 @starryeyez024