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[#51013]: Extend page header with parent link, context bar actions and responsiveness #49

Merged
merged 9 commits into from
Nov 17, 2023
Merged

feat[#51013]: Extend page header with parent link, context bar actions and responsiveness #49

merged 9 commits into from
Nov 17, 2023

Conversation

dominic-braeunlein
Copy link

@dominic-braeunlein dominic-braeunlein commented Nov 14, 2023 •
edited
Loading

What are you trying to accomplish?

This PR extends page header with parent link, context bar actions and responsiveness.

Screenshots

Screenshot with parent link and context action bar Screenshot with back button and breadcrumbs

Integration

no

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.

Accessibility

  • Fixes axe scan violation - This change fixes an existing axe scan violation.
  • No new axe scan violation - This change does not introduce any new axe scan violations.
  • New axe violation - This change introduces a new axe scan violation. Please describe why the violation cannot be resolved below.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 14, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 841f449

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@openproject/primer-view-components Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 14, 2023 •
edited
Loading

⚠️ Visual differences found

Our visual comparison tests found UI differences. Please review the differences by viewing the files changed tab to ensure that the changes were intentional.

Review visual differences

@dominic-braeunlein dominic-braeunlein force-pushed the 51013-add-parent-link-actions-and-responsiveness branch from f71e329 to e4608e0 Compare November 14, 2023 18:34
@dominic-braeunlein dominic-braeunlein marked this pull request as ready for review November 14, 2023 18:43
@dominic-braeunlein dominic-braeunlein force-pushed the 51013-add-parent-link-actions-and-responsiveness branch from dac4758 to 261ce7b Compare November 15, 2023 06:33
@dominic-braeunlein dominic-braeunlein force-pushed the 51013-add-parent-link-actions-and-responsiveness branch from 261ce7b to be7884d Compare November 15, 2023 06:36
HDinger
HDinger reviewed Nov 15, 2023
View reviewed changes
Copy link
Collaborator

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

Hi @dominic-braeunlein

looks great! The automatic switch between the breadcrumb and the parent link is so cool 🥳 I only have very minor remarks, mostly with reagrds to how we comunicate that the parent link and the context bar actions are mobile only.

app/components/primer/open_project/page_header.html.erb Outdated Show resolved Hide resolved
app/components/primer/open_project/page_header.rb Show resolved Hide resolved
previews/primer/open_project/page_header_preview.rb Outdated Show resolved Hide resolved
previews/primer/open_project/page_header_preview.rb Outdated Show resolved Hide resolved
@dominic-braeunlein
chore[#51013]: Change margin strategy to avoid different heights
a864912 
- Removes extra y-margin on action buttons to avoid higher title bar height when buttons are rendered
- Remove margin from contextArea to avoid space when context elements are defined but not rendered because of responsiveness.
- Gives more vertical space for breadcrumbs based on design
@dominic-braeunlein
chore[#51013]: Rename containers to contextBar and PageHeader-titleBar
7a97caf
@dominic-braeunlein
fix[#51013]: Move context bar actions to the end for all combinations
c427d6f
@dominic-braeunlein
fix[#51013]: Add responsiveness for context bar actions
7701468
@dominic-braeunlein
chore[#51013]: Add notes in previews and more descriptive labels
b71a3e9
@dominic-braeunlein
chore[#51013]: Add context bar actions preview example
12e4787
@dominic-braeunlein
chore[#51013]: Add responsive description on slots
ac78f51
@dominic-braeunlein
chore[#51013]: Add actions to playground and switch to template
841f449
@opf opf deleted a comment from github-actions bot Nov 16, 2023
@opf opf deleted a comment from github-actions bot Nov 16, 2023
@dominic-braeunlein
Copy link
Author

dominic-braeunlein commented Nov 16, 2023 •
edited
Loading

Hi @HDinger I updated the PR.

Work done since your last review:

  • Renamed title container back to titleBar.
  • Added comments regarding responsive display argument for the slots in page_header.rb.
  • Added notes regarding display argument for lookbook in page_header_preview.rb.
  • Renamed example labels with narrow and wide for examples in page_header_preview.rb.
  • Added optional actions and context_bar_actions in the playground wich is now in a template file.
  • Removed the 2px top and bottom margin of the actions area to avoid height differences when actions are rendered.

Let me know if you have any additional input and thank you for your review!

HDinger
HDinger approved these changes Nov 17, 2023
View reviewed changes
Copy link
Collaborator

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

Lgtm 👍 great job!

@HDinger HDinger merged commit 5ba81bb into main Nov 17, 2023
28 checks passed
@HDinger HDinger deleted the 51013-add-parent-link-actions-and-responsiveness branch November 17, 2023 08:01
@openprojectci openprojectci mentioned this pull request Nov 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HDinger HDinger HDinger approved these changes

Assignees
No one assigned
Labels
css minor release ruby
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dominic-braeunlein @HDinger