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

Explicit block decoration ordering #18773

Merged
merged 8 commits into from Feb 2, 2019

Conversation

Projects
None yet
1 participant
@smashwilson
Copy link
Member

smashwilson commented Feb 1, 2019

Requirements for Adding, Changing, or Removing a Feature

  • Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • The pull request must contribute a change that has been endorsed by the maintainer team. See details in the template below.
  • The pull request must update the test suite to exercise the updated functionality. For guidance, please see https://flight-manual.atom.io/hacking-atom/sections/writing-specs/.
  • After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see https://github.com/atom/atom/tree/master/CONTRIBUTING.md#pull-requests.

Issue or RFC Endorsed by Atom's Maintainers

Implementation of RFC-004: Decoration ordering.

Description of the Change

Order co-located block decorations by an optional order parameter, or, if unspecified or in the event of ties, the creation order of the decorations (oldest to newest).

I also touched up the documentation for TextEditor::decorateMarker - we had two redundant (and inconsistent!) lists of type parameter values.

Alternate Designs

Rather than .sort() after each insertion, I could have maintained the decorations Array as an ordered list and used a binary search to insert each added decoration. I reasoned that using the built-in, native sort would likely be faster and less bug-prone than anything I could hand-roll.

I could have also avoided adding id and order to Decoration properties by copying the properties object and adding them to the copy instead. I chose to add them directly instead to prevent the creation of a new object for every added Decoration which would need to be GC'ed and so on. Probably micro-optimization but fairly low-risk.

Possible Drawbacks

Co-located block decorations with {position: "after"} previously appeared newest to oldest; now they appear newest to oldest.

Verification Process

I verified that this works as expected by implementing it in tandem with atom/github#1913, which relies on this functionality to order the file patch headers in a MultiFilePatchView when they're all collapsed.

Release Notes

  • Co-located block decorations use an "order" parameter to control the order in which they occur in the DOM.

smashwilson added some commits Feb 1, 2019

smashwilson added a commit to atom/github that referenced this pull request Feb 1, 2019

@smashwilson smashwilson merged commit 8d8e39d into master Feb 2, 2019

3 checks passed

Atom Pull Requests #20190201.7 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@smashwilson smashwilson deleted the aw/block-decoration-order branch Feb 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.