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.

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

feat: Screen Resolution Abstraction #1598

Merged
merged 23 commits into from
Aug 8, 2020
Merged

feat: Screen Resolution Abstraction #1598

merged 23 commits into from
Aug 8, 2020

Conversation

eonarheim
Copy link
Member

@eonarheim eonarheim commented Jul 2, 2020 •
edited
Loading

===:clipboard: PR Checklist :clipboard:===

  • 📌 issue exists in github for these changes
  • 🔬 existing tests still pass
  • 🙈 code conforms to the style guide
  • 📐 new tests written and passing / old tests updated with new scenario(s)
  • 📄 changelog entry added (or not needed)

==================

Closes #1617

Changes:

  • Refactors and creates a screen abstraction for managing all things resolution & viewport related logic
  • Adds and fixes tests for the screen abstraction

@eonarheim eonarheim added the feature Label applied to new feature requests label Jul 2, 2020
@github-actions github-actions bot added the enhancement Label applied to enhancements or improvements to existing features label Jul 2, 2020
@eonarheim eonarheim force-pushed the feature/screen-resolution-abstraction branch from d423d18 to ff28561 Compare July 2, 2020 19:54
kamranayub
kamranayub reviewed Jul 3, 2020
View reviewed changes
Copy link
Member

@kamranayub kamranayub left a comment

Choose a reason for hiding this comment

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

Looking good.

Some questions around:

  1. Using resolution instead of viewport
  2. Cleaning up event handlers

src/engine/Engine.ts Outdated Show resolved Hide resolved
src/engine/Engine.ts Outdated Show resolved Hide resolved
src/engine/Engine.ts Outdated Show resolved Hide resolved
src/engine/Screen.ts Outdated Show resolved Hide resolved
src/engine/Screen.ts Outdated Show resolved Hide resolved
kamranayub
kamranayub approved these changes Jul 19, 2020
View reviewed changes
Copy link
Member

@kamranayub kamranayub left a comment

Choose a reason for hiding this comment

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

I like it

version.js
@@ -19,7 +19,8 @@ function getCurrentCommit() {
}

function getLatestTag(commit) {
execSync('git fetch');
execSync('git fetch --depth=1000');
Copy link
Member

Choose a reason for hiding this comment

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

Issue getting all the tags? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Temporary fix, Travis does a shallow clone :(

Copy link
Contributor

@djcsdy djcsdy Oct 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You can get a deep clone on Travis by adding git fetch --unshallow to your before_script:

before_script:
  - git fetch --unshallow

Edit: I see that's essentially what you're doing above. Never mind. Sorry for the noise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I might switch to this. Seems far less brittle

@eonarheim eonarheim marked this pull request as ready for review August 8, 2020 01:53
@eonarheim eonarheim merged commit 37f1a57 into master Aug 8, 2020
@eonarheim eonarheim deleted the feature/screen-resolution-abstraction branch August 8, 2020 02:01
@eonarheim eonarheim restored the feature/screen-resolution-abstraction branch August 8, 2020 02:01
@eonarheim eonarheim deleted the feature/screen-resolution-abstraction branch August 8, 2020 02:02
@eonarheim eonarheim restored the feature/screen-resolution-abstraction branch August 8, 2020 02:02
@eonarheim eonarheim deleted the feature/screen-resolution-abstraction branch August 8, 2020 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djcsdy djcsdy djcsdy left review comments

@kamranayub kamranayub kamranayub approved these changes

Assignees
No one assigned
Labels
enhancement Label applied to enhancements or improvements to existing features feature Label applied to new feature requests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor Screen Logic to Support Independent Viewport and Resolution
3 participants
@eonarheim @djcsdy @kamranayub