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

Tips views: Adjusting the tips views #78

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Tips views: Adjusting the tips views #78

wants to merge 1 commit into from

Conversation

orIsrael
Copy link
Collaborator

@orIsrael orIsrael commented Apr 18, 2022 •
edited
Loading

Dependent on Yoav's PR #71
screen shots:
image
image
image

@orIsrael orIsrael changed the title Tips views Tips views: Adjusting the tips views Apr 18, 2022
@orIsrael orIsrael force-pushed the tips_views branch 2 times, most recently from 2618422 to a0741e7 Compare April 19, 2022 05:29
@ronyyosef
Copy link
Collaborator

Can you please squash the three last commits? Its realy hard to review like this because we cant see only the changes that you made in this PR

@orIsrael
Copy link
Collaborator Author

an you please squash the three last commits? Its realy hard to review like this because we cant see only the changes that you made in this PR

I think the changes should be in different commits

ronyyosef
ronyyosef suggested changes Apr 19, 2022
View reviewed changes
Copy link
Collaborator

@ronyyosef ronyyosef left a comment

Choose a reason for hiding this comment

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

Left some comments, please fix them and I will do a review again after

@LiorKGOW
Copy link
Collaborator

Is this PR dependent on another PR? #71 ?
If so, please add the appropriate label, and mention it at the top comment of this PR

@orIsrael
Copy link
Collaborator Author

I don't have the option I will ask from the mentors

@LiorKGOW
Copy link
Collaborator

I don't have the option I will ask from the mentors

I can add it till you get the access
Do you want me to add another label?

@LiorKGOW LiorKGOW added the Blocked by another PR Blocked by another PR label Apr 19, 2022
@Yoavsc129
Copy link
Collaborator

This PR isn't based on the latetst version of mine (#71). Be aware that the tips/detail.html page was removed and that the text of the tips should appear in tips/board.html

@Yoavsc129 Yoavsc129 mentioned this pull request Apr 19, 2022
Merged
@orIsrael orIsrael force-pushed the tips_views branch 4 times, most recently from 4d7ec56 to a7fb69e Compare April 21, 2022 16:55
@Yoavsc129
Copy link
Collaborator

Please squash all unnecessary commits

Yoavsc129
Yoavsc129 suggested changes Apr 21, 2022
View reviewed changes
Copy link
Collaborator

@Yoavsc129 Yoavsc129 left a comment

Choose a reason for hiding this comment

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

Some of the html files added already exist. Please make sure all the relevant files are in templates/tips and delete
tips/templates/tips

ShellyGolden
ShellyGolden reviewed Apr 22, 2022
View reviewed changes
@orIsrael
Copy link
Collaborator Author

Please squash all unnecessary commits

I think all of the commits are necessary

@orIsrael
Copy link
Collaborator Author

Please squash all unnecessary commits

Can you be more specific?

@orIsrael orIsrael force-pushed the tips_views branch 2 times, most recently from 3f02860 to 3050e9e Compare May 26, 2022 10:13
@ronyyosef
Copy link
Collaborator

ronyyosef commented May 26, 2022 •
edited
Loading

Can you change the status to draft?

@LiorKGOW
Copy link
Collaborator

@orIsrael I saw you have added the In-Progress label.
Please update in Slack when this PR is ready for review , or if you need any help with it👍

@orIsrael orIsrael force-pushed the tips_views branch 3 times, most recently from 39d9c1e to 1fcec55 Compare May 28, 2022 13:06
ShellyGolden
ShellyGolden reviewed May 28, 2022
View reviewed changes
@orIsrael orIsrael force-pushed the tips_views branch 2 times, most recently from 5703927 to 20d1b01 Compare May 28, 2022 16:14
ronyyosef
ronyyosef suggested changes May 28, 2022
View reviewed changes
Copy link
Collaborator

@ronyyosef ronyyosef left a comment

Choose a reason for hiding this comment

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

Hey @orIsrael, I left some comments.
It looks like a lot of things that are not part of this PR are in it, Can you check and remove them from this PR?

ronyyosef
ronyyosef reviewed May 29, 2022
View reviewed changes
Pipfile
@@ -12,6 +12,7 @@ django-crispy-forms = "==1.14.0"
django-material = "*"
django-smart-selects = {editable = true, ref = "master", git = "https://github.com/jazzband/django-smart-selects.git"}
pre-commit = "*"
pytest = "*"
Copy link
Collaborator

@ronyyosef ronyyosef May 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We pytest in the dev-packges section

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shlior7 shlior7 shlior7 left review comments

@ronyyosef ronyyosef ronyyosef requested changes

@LiorKGOW LiorKGOW LiorKGOW requested changes

@ShellyGolden ShellyGolden ShellyGolden left review comments

@svatares svatares Awaiting requested review from svatares

@Yoavsc129 Yoavsc129 Awaiting requested review from Yoavsc129

Assignees

@orIsrael orIsrael

Labels
Ready for a review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@orIsrael @ronyyosef @LiorKGOW @Yoavsc129 @svatares @shlior7 @ShellyGolden @adiabramovitch