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

refactor: fix padding #445

Merged
merged 4 commits into from
Nov 30, 2024
Merged

refactor: fix padding #445

merged 4 commits into from
Nov 30, 2024

Conversation

tomasz-trela
Copy link
Member

@tomasz-trela tomasz-trela self-assigned this Nov 29, 2024
simon-the-shark
simon-the-shark reviewed Nov 29, 2024
View reviewed changes
lib/config/ui_config.dart Outdated
@@ -195,7 +195,7 @@ abstract class SksConfig {
static const sizedBoxWidth = 5.0;
static const radius = 8.0;
static const innerPadding = EdgeInsets.symmetric(horizontal: 8, vertical: 4);
static const outerPadding = EdgeInsets.only(right: 12, bottom: 2);
static const outerPadding = EdgeInsets.only(right: 24, bottom: 2);
Copy link
Member

Choose a reason for hiding this comment

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

did you check that this padding isn't used in any other place?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I didn't check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it is used in sks menu tab in SksUserButton. But I can't checkout it because sks menu is offline
Simulator Screenshot - iPhone 16 Plus - 2024-11-29 at 20 43 03

Copy link
Member

Choose a reason for hiding this comment

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

try to comment condition that checks if menu is offline. We always have some kind of data (even old).

if (!sksMenuData.isMenuOnline) {
     return const _SKSMenuLottieAnimation();
   }

line 61 of sks_menu_screen.dart

@tomasz-trela
Copy link
Member Author

So we have to wait till Monday :/

mikolaj-jalocha
mikolaj-jalocha reviewed Nov 30, 2024
View reviewed changes
Copy link
Member

@mikolaj-jalocha mikolaj-jalocha left a comment

Choose a reason for hiding this comment

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

Please check if paddings didn't disturb other parts of UI. I left one comment regarding how to view sks's data even if menu's offline.

lib/config/ui_config.dart Outdated
@@ -195,7 +195,7 @@ abstract class SksConfig {
static const sizedBoxWidth = 5.0;
static const radius = 8.0;
static const innerPadding = EdgeInsets.symmetric(horizontal: 8, vertical: 4);
static const outerPadding = EdgeInsets.only(right: 12, bottom: 2);
static const outerPadding = EdgeInsets.only(right: 24, bottom: 2);
Copy link
Member

Choose a reason for hiding this comment

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

try to comment condition that checks if menu is offline. We always have some kind of data (even old).

if (!sksMenuData.isMenuOnline) {
     return const _SKSMenuLottieAnimation();
   }

line 61 of sks_menu_screen.dart

@tomasz-trela
Copy link
Member Author

But I've got an exception
Zrzut ekranu 2024-11-30 o 15 47 36
So commenting this code gives no result

@mikolaj-jalocha
Copy link
Member

But I've got an exception Zrzut ekranu 2024-11-30 o 15 47 36 So commenting this code gives no result

that's weird. I'll check on that

@simon-the-shark simon-the-shark added the bug Something isn't working label Nov 30, 2024
@simon-the-shark simon-the-shark linked an issue Nov 30, 2024 that may be closed by this pull request
fix/fix-padding-at-last-tab #444
Closed
@mikolaj-jalocha
Copy link
Member

But I've got an exception Zrzut ekranu 2024-11-30 o 15 47 36 So commenting this code gives no result

the oldest joke in the industry, but work on my computer XD

image

it's the only thing that I've done.

@tomasz-trela
Copy link
Member Author

Simulator Screenshot - iPhone 16 Plus - 2024-11-30 at 20 33 31
So this screen looks like this and I would do separate value of padding these screen.

@simon-the-shark
Copy link
Member

Simulator Screenshot - iPhone 16 Plus - 2024-11-30 at 20 33 31
So this screen looks like this and I would do separate value of padding these screen.

Yes, lets separate it

@tomasz-trela
Copy link
Member Author

Simulator Screenshot - iPhone 16 Plus - 2024-11-30 at 20 43 14
Now it looks like it was previously

@simon-the-shark simon-the-shark force-pushed the feat/fix-padding-at-last-tab branch from e46a969 to 79afe23 Compare November 30, 2024 20:41
@simon-the-shark simon-the-shark merged commit 4cb397f into main Nov 30, 2024
2 checks passed
@simon-the-shark simon-the-shark deleted the feat/fix-padding-at-last-tab branch November 30, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simon-the-shark simon-the-shark simon-the-shark approved these changes

@mikolaj-jalocha mikolaj-jalocha Awaiting requested review from mikolaj-jalocha

Assignees

@tomasz-trela tomasz-trela

Labels
bug Something isn't working
Projects
None yet
Milestone
v1.2.0 - Mr. Shark, We Have to Cook
Development

Successfully merging this pull request may close these issues.

fix/fix-padding-at-last-tab
3 participants
@tomasz-trela @mikolaj-jalocha @simon-the-shark