Skip to content

Update header UI#4050

Merged
daviddossett merged 9 commits intomainfrom
ddossett/header-updates
Oct 14, 2022
Merged

Update header UI#4050
daviddossett merged 9 commits intomainfrom
ddossett/header-updates

Conversation

@daviddossett
Copy link
Contributor

@daviddossett daviddossett commented Oct 13, 2022

This PR addresses the following:

  • Updates header styles and layout to match the design in Polish Extension UI #4029 and aligns with extension detail page in core
  • Uses extension detail- like buttons instead of mixed icons/large buttons
  • Ensures content reflows on small viewport/editor sizes
  • Updates max width of overview content
  • Refactors header JSX by extracting out Title, Subtitle, and ButtonGroup into their own components.

Before

CleanShot 2022-10-13 at 11 16 32@2x

After

CleanShot 2022-10-13 at 11 05 35@2x

Demo

CleanShot.2022-10-13.at.11.06.43.mp4

@alexr00 I didn't have the time but I think we should replace the title editing form with a quick pick like how Settings Profile rename works. Thoughts?

cc @digitarald

rzhao271
rzhao271 previously approved these changes Oct 13, 2022
@Thomas1664
Copy link
Contributor

A visual distinction between drafts and open PRs like on the GitHub website would be nice, i.e. by making the open label green

@Thomas1664
Copy link
Contributor

@daviddossett In #4052 I informed you that your PR breaks icon button focus outline, but you didn't take my concern serious: #4052 (comment)

After merging your changes from main into #4057 I noticed that my previously added support for focus is no longer visible. I hope that you take my concern seriously now and fix the issue that was caused by your PR.

@daviddossett daviddossett merged commit 837b6fb into main Oct 14, 2022
@daviddossett daviddossett deleted the ddossett/header-updates branch October 14, 2022 23:06
@daviddossett
Copy link
Contributor Author

@Thomas1664 where are you still seeing the issue? It sounded like it was fixed by this PR removing the icons from the header.

@Thomas1664
Copy link
Contributor

@Thomas1664 where are you still seeing the issue? It sounded like it was fixed by this PR removing the icons from the header.

In the comment action bar. But you have to check out #4057 because the action buttons weren't visible before on focus.

@daviddossett
Copy link
Contributor Author

I already fixed it in #4066

@alexr00
Copy link
Member

alexr00 commented Oct 17, 2022

I didn't have the time but I think we should replace the title editing form with a quick pick like how Settings Profile rename works. Thoughts?

I have a slight preference for leaving it as is. When you edit a comment in the overview, you get the text area. Keeping the title the same way is consistent.

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.

5 participants