Skip to content

ensure the old layout is fine for users until they see the new merge hint#5563

Merged
outofambit merged 1 commit intodisplay-merge-status-in-compare-tabfrom
ensure-the-old-layout-is-fine
Sep 7, 2018
Merged

ensure the old layout is fine for users until they see the new merge hint#5563
outofambit merged 1 commit intodisplay-merge-status-in-compare-tabfrom
ensure-the-old-layout-is-fine

Conversation

@shiftkey
Copy link
Member

@shiftkey shiftkey commented Sep 7, 2018 •
edited
Loading

This PR ensures the existing message (which will be default until we enable the feature flag) still looks fine.

To test this out, checkout display-merge-status-in-compare-tab and disable the feature flag:

diff --git a/app/src/lib/feature-flag.ts b/app/src/lib/feature-flag.ts
index 1572bf5e0..7b46277b5 100644
--- a/app/src/lib/feature-flag.ts
+++ b/app/src/lib/feature-flag.ts
@@ -44,7 +44,7 @@ export function enableRepoInfoIndicators(): boolean {
 
 /** Should the app try and detect conflicts before the user stumbles into them? */
 export function enableMergeConflictDetection(): boolean {
-  return enableDevelopmentFeatures()
+  return false
 }
 
 /** Should `git status` use --no-optional-locks to assist with concurrent usage */

Run it up locally and you'll see it looks more squashed compared to what we've currently shipped:

screen shot 2018-09-07 at 10 00 45 am

This PR adds back that top margin, and indicates it can be cleaned up when we're happy with this feature:

screen shot 2018-09-07 at 10 04 16 am

cc @desktop/design for 👀 and ideas on how to manage styles that are tied to feature flags like this.

@shiftkey shiftkey added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 7, 2018
@shiftkey shiftkey added this to the 1.4.0 milestone Sep 7, 2018
@outofambit
Copy link
Contributor

outofambit commented Sep 7, 2018 •
edited
Loading

i would love to move some (or more) of our styles to use https://www.styled-components.com/ (or something simliar) at some point as that would make conditional styling like this much simpler

@outofambit outofambit self-assigned this Sep 7, 2018
@outofambit outofambit requested a review from donokuda September 7, 2018 15:50
donokuda
donokuda approved these changes Sep 7, 2018
View reviewed changes
Copy link
Contributor

@donokuda donokuda left a comment

Choose a reason for hiding this comment

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

cc @desktop/design for 👀 and ideas on how to manage styles that are tied to feature flags like this.

One approach I can think of off the top of my head is to remove the margin-top: var(--spacing) on the merge message container and add a margin-bottom: var(--spacing) to the merge button (or the ButtonGroup that wraps the button.)

Additionally, I am in favor of exploring @outofambit's suggestion for situations like this.

@outofambit outofambit merged commit 760f827 into display-merge-status-in-compare-tab Sep 7, 2018
@outofambit outofambit deleted the ensure-the-old-layout-is-fine branch September 7, 2018 17:03
@j-f1
Copy link
Contributor

j-f1 commented Sep 8, 2018

@outofambit Related: React: CSS in your JS by Christopher Chedeau

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@outofambit outofambit outofambit approved these changes

@donokuda donokuda donokuda approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@outofambit outofambit

Labels

ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Milestone

1.4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@shiftkey @outofambit @j-f1 @donokuda