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

[FullscreenBar] Add this component for use in Fullscreen apps #5688

Merged
merged 2 commits into from
May 20, 2022
Merged

[FullscreenBar] Add this component for use in Fullscreen apps #5688

merged 2 commits into from
May 20, 2022

Conversation

camd
Copy link
Contributor

@camd camd commented May 2, 2022 •
edited

WHY are these changes introduced?

Fixes #5664

We are rolling out Fullscreen mode in Apps (first and third-party) in App Bridge. We want to give App developers an easy way to be conformant with our requirements on Fullscreen. If they are in that mode, they MUST provide a navigation bar with a back button to exit.

WHAT is this pull request doing?

This PR adds a top bar with uniformed styling on a Back button to exit Fullscreen mode. It can contain children so that the App developer can customize the rest of the bar to their needs.

Fullscreen bar - Fullscreen bar with children ⋅ Storybook 2022-05-02 16-26-04

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

You can test this component locally in Storybook:
Fullscreen bar

or in a Spin instance: https://shop1.shopify.fsbar.cameron-dawson.us.spin.dev/admin/email_templates/order_confirmation/edit

  • Check toward the top of the page to see it. Click the Back button for a Toast to pop up.

🎩 checklist

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2022 •
edited

size-limit report 📦

Path Size
cjs 151.01 KB (+0.1% 🔺)
esm 87.36 KB (+0.39% 🔺)
esnext 137.37 KB (+0.34% 🔺)
css 37.31 KB (+0.22% 🔺)

@camd camd force-pushed the camd/fullscreenbar branch 2 times, most recently from 82d8495 to be1bb0e Compare May 2, 2022 19:08
kyledurand
kyledurand reviewed May 2, 2022
View reviewed changes
Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Couple of comments but this is looking good 👍

polaris-react/src/components/FullscreenBar/FullscreenBar.scss Outdated Show resolved Hide resolved
polaris-react/src/components/FullscreenBar/FullscreenBar.scss Outdated Show resolved Hide resolved
polaris-react/src/components/FullscreenBar/FullscreenBar.scss Outdated Show resolved Hide resolved
polaris-react/src/components/FullscreenBar/README.md Outdated Show resolved Hide resolved
polaris-react/src/components/FullscreenBar/tests/FullscreenBar.test.tsx Outdated Show resolved Hide resolved
polaris-react/src/components/FullscreenBar/FullscreenBar.scss Show resolved Hide resolved
@camd
Copy link
Contributor Author

camd commented May 2, 2022

I wanted to ask your advice on a larger question about this component in relation to App Bridge apps. I figure Polaris is intended to be agnostic to App Bridge, so I shouldn't mention it here. But I will mention this component from the App Bridge side. Does that sound like the right approach? Or could /should I add a link in the README here pointing to App Bridge?

@camd camd mentioned this pull request May 2, 2022
Closed
@camd camd changed the title Camd/fullscreenbar [FullscreenBar] Add this component for use in Fullscreen apps May 2, 2022
@camd camd marked this pull request as ready for review May 2, 2022 23:42
@camd camd force-pushed the camd/fullscreenbar branch 2 times, most recently from 0081c22 to f0057c2 Compare May 3, 2022 23:43
@camd camd requested a review from kyledurand May 3, 2022 23:43
@camd camd self-assigned this May 3, 2022
kyledurand
kyledurand reviewed May 4, 2022
View reviewed changes
Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Almost good to go but would like some more eyes if we can get them. Going to request @jjgali for content as well

polaris-react/src/components/FullscreenBar/FullscreenBar.scss Outdated Show resolved Hide resolved
polaris-react/src/components/FullscreenBar/FullscreenBar.scss Outdated Show resolved Hide resolved
polaris-react/src/components/FullscreenBar/README.md Outdated Show resolved Hide resolved
polaris-react/src/components/FullscreenBar/tests/FullscreenBar.test.tsx Outdated Show resolved Hide resolved
@kyledurand kyledurand requested a review from jjgali May 4, 2022 17:55
jjgali
jjgali reviewed May 4, 2022
View reviewed changes
polaris-react/src/components/FullscreenBar/README.md Outdated
## Related components

- To provide quick, at-a-glance feedback on the outcome of an action, use the [toast](https://polaris.shopify.com/components/feedback-indicators/toast) component.
- To indicate to merchants that a page is loading or an upload is processing use the [loading](https://polaris.shopify.com/components/feedback-indicators/loading) component.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comma after "processing":

To indicate to merchants that a page is loading or an upload is processing, use the ...

jjgali
jjgali reviewed May 4, 2022
View reviewed changes
Copy link
Contributor

@jjgali jjgali left a comment •
edited

Choose a reason for hiding this comment

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

Left a few comments—I think the main things we'll want to fix, content wise, are references to the top bar component that got copied over, as well as the main description. I think we should rewrite it so it's not a direct copy of the top bar description.

Happy to take another look when you're ready!

@camd
Copy link
Contributor Author

camd commented May 4, 2022

Thanks for the great review! Sorry I flubbed on the README. Dang. OK, I've fixed the references. Please take another look at your leisure.

@camd camd requested a review from jjgali May 4, 2022 21:30
jjgali
jjgali approved these changes May 4, 2022
View reviewed changes
Copy link
Contributor

@jjgali jjgali left a comment

Choose a reason for hiding this comment

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

Changes look great on my end! Also fixed a few tiny grammar/punctuation things in the readme directly, if that's okay.

@camd
Copy link
Contributor Author

camd commented May 5, 2022

Changes look great on my end! Also fixed a few tiny grammar/punctuation things in the readme directly, if that's okay.

Thank you! Yeah, that's great and much appreciated. 😄

@camd
Copy link
Contributor Author

camd commented May 11, 2022

Hey folks-- Are there more reviews required for this to go through? Looks like several people are assigned. I'll rebase today, but the conflict looks constrained to UNRELEASED.md, so no functionality differences.

Thanks in advance!!

kyledurand
kyledurand approved these changes May 13, 2022
View reviewed changes
polaris-react/src/components/FullscreenBar/README.md Outdated
Comment on lines 5 to 6
- global chrome
- global features
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering if this is truly global. We may want to omit these since they're not used on every page in the admin

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually there are probably a few more here that should be removed as well since they're not directly related to this component:

  • nav bar
  • navbar
  • brand
  • search
  • user
  • menu
  • logo

Can probably all be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks!! Yeah, I really wasn't sure what was right there, so this really helps.

@changeset-bot
Copy link

changeset-bot bot commented May 13, 2022 •
edited

⚠️ No Changeset found

Latest commit: e8e0086

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@camd
Copy link
Contributor Author

camd commented May 16, 2022

@kyledurand : There's a changeset-bot that's recommending I add one. But it looks like we add an entry to the UNRELEASED changelog instead, is that right? Or should I create a changeset?

If that's all good, is this good to merge now? 😄

@alex-page
Copy link
Member

@camd Ignore the changeset for now

@kyledurand
Copy link
Contributor

🚀

@camd camd requested review from lucabezerra and removed request for lucabezerra May 19, 2022 17:13
@camd camd force-pushed the camd/fullscreenbar branch 2 times, most recently from 403a9d3 to 34aaa01 Compare May 19, 2022 20:28
@alex-page
Copy link
Member

@camd we have shipped changeset now. Please run yarn changeset before shipping this pull request.

@camd camd force-pushed the camd/fullscreenbar branch 3 times, most recently from abf27e9 to 4c42ad9 Compare May 19, 2022 21:45
@camd camd merged commit c66352b into main May 20, 2022
@camd camd deleted the camd/fullscreenbar branch May 20, 2022 17:09
@ghost
Copy link

ghost commented May 20, 2022

🎉 Thanks for your contribution to Polaris!

This was referenced May 20, 2022
Merged
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyledurand kyledurand kyledurand approved these changes

@alex-page alex-page alex-page approved these changes

@jjgali jjgali jjgali approved these changes

@aaronccasanova aaronccasanova Awaiting requested review from aaronccasanova

@sam-b-rose sam-b-rose Awaiting requested review from sam-b-rose

@lgriffee lgriffee Awaiting requested review from lgriffee

@tmlayton tmlayton Awaiting requested review from tmlayton

Assignees

@camd camd

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create new FullscreenBar component
4 participants
@camd @alex-page @kyledurand @jjgali