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.

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

[No QA] Remove all inline named exports. Update style guide. #6013

Merged
merged 4 commits into from
Oct 23, 2021

Conversation

marcaaron
Copy link
Contributor

cc @roryabraham

Details

Removes all inline named exports

Fixed Issues

$ #6011

Tests / QA Steps

  • Built storybook and ran tests to make sure changing those named exports did not cause issues.
  • No specific test steps. Just normal regressions. Make sure app builds OK etc.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Oct 22, 2021
@marcaaron marcaaron requested a review from a team as a code owner October 22, 2021 23:50
@MelvinBot MelvinBot requested review from aldo-expensify and removed request for a team October 22, 2021 23:51
Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

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

Project built fine and looks fine.

NAB: Since all exports should be at the bottom of the file, including default, we should update other places of the documentation like (maybe is the only one :) ):

image

Update: Changed my comment into a NAB because I see that the code in all xx.stories.js don't have the default export at the end. Maybe you are intentionally leaving it out of the refactor?

STORYBOOK.md Outdated Show resolved Hide resolved
STYLE.md Outdated Show resolved Hide resolved
@marcaaron
Copy link
Contributor Author

NAB: Since all exports should be at the bottom of the file, including default, we should update other places of the documentation like (maybe is the only one :) ):

Honestly, I skimmed about half of the files and didn't see any so I think we can fix them as we go.
Will fix the comment about the semi-colon.

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Oct 23, 2021

NAB: Since all exports should be at the bottom of the file, including default, we should update other places of the documentation like (maybe is the only one :) ):

Honestly, I skimmed about half of the files and didn't see any so I think we can fix them as we go. Will fix the comment about the semi-colon.

It's fine, it can be fixed as we go, as you say. In case you are curious, this is one of the files I meant:

export default {
title: 'Components/HeaderWithCloseButton',
component: HeaderWithCloseButton,
};

aldo-expensify
aldo-expensify previously approved these changes Oct 23, 2021
Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

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

Looks good to me, it builds fine and I didn't notice anything no working in the app.

Leaving it un-merged in case @roryabraham wants to review.

@marcaaron
Copy link
Contributor Author

Went through them all. I think we got em! 😄

Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

LGTM

@roryabraham roryabraham merged commit 2fccbd2 into main Oct 23, 2021
@roryabraham roryabraham deleted the marcaaron-noInlineNamedExport branch October 23, 2021 16:39
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @roryabraham in version: 1.1.8-14 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@roryabraham roryabraham changed the title Remove all inline named exports. Update style guide. [No QA] Remove all inline named exports. Update style guide. Oct 25, 2021
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

4 participants