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

[#6422] pagination issues fixes #4513

Merged
merged 3 commits into from
Sep 29, 2022
Merged

[#6422] pagination issues fixes #4513

merged 3 commits into from
Sep 29, 2022

Conversation

khamui
Copy link
Contributor

@khamui khamui commented Sep 26, 2022

No description provided.

Copy link
Contributor

@goapunk goapunk left a comment

Choose a reason for hiding this comment

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

nice fixes! just one small thing

@khamui khamui changed the title [#6422] pagination issues fixes WIP [#6422] pagination issues fixes Sep 27, 2022
@khamui khamui force-pushed the kt-2022-09-issues-fixes branch from 4f8073d to cb06992 Compare September 27, 2022 09:19
@khamui khamui changed the title WIP [#6422] pagination issues fixes [#6422] pagination issues fixes Sep 27, 2022
@khamui
Copy link
Contributor Author

khamui commented Sep 27, 2022

just realize #4510 is just fixed for the react pagination, still the django pagination needs to be done, but not sure if this is quite a bit of rework (at least for me).

done both for react and django

@khamui khamui requested a review from goapunk September 27, 2022 09:53
Copy link
Contributor

@goapunk goapunk left a comment

Choose a reason for hiding this comment

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

nice, looks good to me!

Copy link
Contributor

@philli-m philli-m left a comment

Choose a reason for hiding this comment

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

few questions :)

meinberlin/apps/budgeting/assets/Pagination.jsx Outdated Show resolved Hide resolved
meinberlin/assets/scss/components/_pagination.scss Outdated Show resolved Hide resolved
@khamui khamui changed the title [#6422] pagination issues fixes WIP [#6422] pagination issues fixes Sep 27, 2022
@khamui khamui force-pushed the kt-2022-09-issues-fixes branch 3 times, most recently from c0a1433 to 3b186fc Compare September 27, 2022 16:00
@khamui khamui changed the title WIP [#6422] pagination issues fixes [#6422] pagination issues fixes Sep 27, 2022
@khamui khamui requested review from philli-m and goapunk September 27, 2022 16:01
@philli-m philli-m self-assigned this Sep 28, 2022
Copy link
Contributor

@philli-m philli-m left a comment

Choose a reason for hiding this comment

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

cool, just a few comments and I just noticed when checking it this time that we don't use BEM, maybe we can change pagination-item to pagination__item?

}
}
}
}
Copy link
Contributor

@philli-m philli-m Sep 28, 2022

Choose a reason for hiding this comment

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

you only need to add the padding change here so all the other styles can be deleted, also as we are trying to adopt mobile first format you can use a min-width query and swap the padding values around. For the padding values, as the smallest screen we ensure design works for is 360, I think the xaxis padding can be a touch bigger to help with button size a11y

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe 0.7 or 0.75 just from having a quick play in the browser but your call

@@ -55,6 +55,7 @@ $font-size-sm: 0.9rem !default;
$font-size-xs: 0.8rem !default;

// use for min-width media-queries
$breakpoint-xxs: 23.75rem !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you want to add an additional breakpoint? also this is added as a min width value but you use it in a max width query, we don't really want to add additional break points as it add complexity, applying the same style up to a width of 512px is pretty standard, bootstraps first breakpoint is 576 so we aren't far off the norm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah, messing up things here! will remove it and use xs instead

@philli-m philli-m removed their assignment Sep 28, 2022
@khamui khamui force-pushed the kt-2022-09-issues-fixes branch from 3b186fc to 6e31e1a Compare September 28, 2022 15:08
@khamui khamui requested a review from philli-m September 28, 2022 15:38
Kha added 2 commits September 28, 2022 16:40
revert mobile version

pagination: fix issues after review
@khamui khamui force-pushed the kt-2022-09-issues-fixes branch from 6e31e1a to ee0929a Compare September 28, 2022 15:41
@fuzzylogic2000
Copy link
Contributor

What is the status here? @khamui It is finished and can be reviewed again? Will you do that @phillimorland ? Or should I? (I am not sure I will notice everything you talked about.)

@philli-m
Copy link
Contributor

@fuzzylogic2000 sorry didn't see the re-tag, will take a look now

@philli-m philli-m self-assigned this Sep 29, 2022
Copy link
Contributor

@philli-m philli-m left a comment

Choose a reason for hiding this comment

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

perfect! :)

@philli-m philli-m merged commit 494925c into main Sep 29, 2022
@philli-m philli-m deleted the kt-2022-09-issues-fixes branch September 29, 2022 11:35
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