Skip to content

UI fixes for PR view#4368

Merged
alexr00 merged 17 commits intomicrosoft:mainfrom
Thomas1664:poish-ui
Jan 12, 2023
Merged

UI fixes for PR view#4368
alexr00 merged 17 commits intomicrosoft:mainfrom
Thomas1664:poish-ui

Conversation

@Thomas1664
Copy link
Contributor

@Thomas1664 Thomas1664 commented Dec 24, 2022

Fixes:

  • PR title and number were not in the same line
  • Space between the buttons for 'Add PR comment' form is too large
  • padding-right for PR labels too small
  • The text of some submit buttons was invisible in light high contrast mode (i.e. Save button for PR comments and Comment button at the bottom of PR webview)
  • Correctly wrap long lines in diffs
  • Fix forms 'Add review summary comment' and 'Add merge comment'

Before:

image

image

image

^ Different PR, no additional buttons are hidden.

image

image

image

image

image

image

After:

image

image

image

image

image

image

image

image

image

@alexr00
Copy link
Member

alexr00 commented Dec 27, 2022

@daviddossett would you like to take this one? I think you may have recently changed some of these things.

@daviddossett
Copy link
Contributor

PR title and number were not in the same line

Any idea how this regressed? This seems to have changed since I last looked.

@daviddossett daviddossett self-requested a review January 10, 2023 00:47
id="reply"
value="Comment"
type="submit"
className="secondary"
Copy link
Contributor

Choose a reason for hiding this comment

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

@alexr00 Not sure I have the full context here—we're these styled secondary for some reason?

Before/after screenshots would be helpful here and for all other UI changes @Thomas1664

Copy link
Member

Choose a reason for hiding this comment

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

I don't actually see a primary class in the css files.

Copy link
Member

Choose a reason for hiding this comment

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

Styling it as secondary appears to have just been to get the alignment correct: #1350

@Thomas1664
Copy link
Contributor Author

@alexr00 @daviddossett I added screenshots for all UI changes

@Thomas1664 Thomas1664 requested review from alexr00 and daviddossett and removed request for alexr00 and daviddossett January 11, 2023 14:25
@alexr00
Copy link
Member

alexr00 commented Jan 11, 2023

@Thomas1664 David and I will work on reviewing this, but for future PRs it would really help if you could make small, targeted, PRs that only fix one thing at a time. It's difficult to tell which of your changes affect which elements.

Copy link
Contributor Author

@Thomas1664 Thomas1664 left a comment

Choose a reason for hiding this comment

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

@alexr00 I reverted some changes that might have affected many things I and added comments on what individual diffs actually fix if there isn't already some annotation provided by the context of the diff.

daviddossett
daviddossett previously approved these changes Jan 12, 2023
Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

Just two small requests!

@alexr00 alexr00 added this to the January 2023 milestone Jan 12, 2023
@Thomas1664 Thomas1664 requested a review from alexr00 January 12, 2023 14:53
@alexr00
Copy link
Member

alexr00 commented Jan 12, 2023

Thank you!

@alexr00 alexr00 merged commit b557e37 into microsoft:main Jan 12, 2023
@Thomas1664 Thomas1664 deleted the poish-ui branch January 12, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants