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

change back button to last viewed page #1522

Open
wants to merge 36 commits into
base: dev
Choose a base branch
from
Open

change back button to last viewed page #1522

wants to merge 36 commits into from

Conversation

varCepheid
Copy link
Collaborator

@varCepheid varCepheid commented Jan 15, 2023 •
edited

closes #1497
This PR takes away the back attribute in the Page element. It also adds a link to the event on event-specific pages and new pages for editing reports separate from the pages to view them.

@varCepheid varCepheid self-assigned this Jan 15, 2023
@varCepheid varCepheid added the enhancement Adds a new feature or meets a request. label Jan 15, 2023
@calebeby
Copy link
Member

@varCepheid The way I designed the "back system" is that the back button goes "up a level", when that makes sense, and when there is no logical "up a level", it will perform the same action as the browser back button (go to the previous page).

Sometimes, that system is confusing. Instead of changing the behavior on this one individual page, can you propose a new logical system for the back button across all pages?

@calebeby
Copy link
Member

I acknowledge that the current system is confusing. Do you think it'd make sense for the back button in all cases to work the same as the browser back button? I would not be opposed to that kind of change.

However, if we did that, then there would be no easy way to, for example, navigate from a match page to the event corresponding to that match, unless that is the page you came from.

What do you think would make the most sense?

@varCepheid
Copy link
Collaborator Author

I think your fix makes more sense, and there should be a link to the event page on the match page.

@varCepheid varCepheid added needs work Changes have been suggested and need to be implemented. priority This needs to be addressed before the current changes can be merged to main. wip and removed needs work Changes have been suggested and need to be implemented. labels Jan 26, 2023
@varCepheid varCepheid added the needs work Changes have been suggested and need to be implemented. label Jan 27, 2023
@calebeby calebeby marked this pull request as draft January 27, 2023 21:46
@varCepheid varCepheid removed the wip label Feb 9, 2023
@varCepheid
Copy link
Collaborator Author

Note to self: finish working on match details card, seems to only be updating on event page; then fix the type errors.

@varCepheid varCepheid added question Further information is requested. and removed needs work Changes have been suggested and need to be implemented. labels Feb 10, 2023
@varCepheid
Copy link
Collaborator Author

@calebeby routes/report, line 51: The back button had stopped editing mode when the report was being edited without changing the page. Now, the back button will always go to the browser's back no matter what. Should there be a button on the report page to stop editing, or is the Save Report button at the bottom good enough?

auto-merge was automatically disabled June 11, 2023 00:20

Pull request was converted to draft

@varCepheid varCepheid marked this pull request as ready for review June 25, 2023 23:42
@varCepheid varCepheid requested a review from calebeby June 25, 2023 23:42
@varCepheid varCepheid removed the needs work Changes have been suggested and need to be implemented. label Aug 2, 2023
calebeby
calebeby requested changes Aug 9, 2023
View reviewed changes
Copy link
Member

@calebeby calebeby left a comment

Choose a reason for hiding this comment

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

Hi @varCepheid! Thanks for your patience with me! I left some feedback. I got a chance to review almost all of the files in this PR, except I haven't had time to closely review the changes to the report editor yet (when big blocks of code are moved the diff view doesn't really show what, if anything was actually changed, so it takes longer to compare)

src/routes/event-match.tsx Outdated Show resolved Hide resolved
src/components/match-card.tsx Outdated Show resolved Hide resolved
src/components/page.tsx Outdated Show resolved Hide resolved
src/routes/report.tsx Outdated Show resolved Hide resolved
src/routes/report.tsx Outdated Show resolved Hide resolved
src/routes/saved-report-edit.tsx Outdated Show resolved Hide resolved
src/routes/saved-report-edit.tsx Outdated Show resolved Hide resolved
src/routes/event-team.tsx Outdated Show resolved Hide resolved
@calebeby calebeby added the needs work Changes have been suggested and need to be implemented. label Aug 9, 2023
@varCepheid
moved headerLinkStyle to Page
5f8c4a9
@varCepheid
reverted changes to match-card
6e1116b
@varCepheid
minor changes
d0642c7 
changed homepage argument of Page & Header to showBackButton and flipped
the logic
changed onEditClick argument of report-viewer to  reportEditorLink and
changed the Button to href
shortened functions in saved-report-edit
@varCepheid
more moving event-page header styles to Page
9458155
@varCepheid
changed colors greenOnPurple and redOnPurple to match accessibility g…
3f90658 
…uidelines
@varCepheid
Copy link
Collaborator Author

Thanks for all the notes! I went with a text-decoration-color attribute for the last point and I think it looks good. I won't merge until you have a chance to look at the report-editor component.

@varCepheid varCepheid added wip and removed needs work Changes have been suggested and need to be implemented. labels Nov 7, 2023
@varCepheid varCepheid removed the wip label Nov 15, 2023
@calebeby calebeby self-requested a review November 19, 2023 06:37
@varCepheid
Copy link
Collaborator Author

@calebeby I've addressed all of the changes you've requested. Can you add another review or approve the changes?

calebeby
calebeby reviewed Feb 10, 2024
View reviewed changes
src/colors.ts Outdated Show resolved Hide resolved
calebeby
calebeby reviewed Feb 10, 2024
View reviewed changes
src/components/page.tsx Outdated Show resolved Hide resolved
calebeby
calebeby requested changes Feb 10, 2024
View reviewed changes
Copy link
Member

@calebeby calebeby left a comment

Choose a reason for hiding this comment

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

Great work here! I still have yet to review the report editor changes, I can't remember why those are in this PR and I don't have time to look closely right now.

src/routes/event-analysis.tsx Outdated Show resolved Hide resolved
src/routes/event-match.tsx Outdated Show resolved Hide resolved
src/routes/event-match.tsx Outdated Show resolved Hide resolved
src/routes/event-team-comments.tsx Outdated Show resolved Hide resolved
src/routes/event-team-matches.tsx Outdated Show resolved Hide resolved
src/routes/event-team.tsx Outdated Show resolved Hide resolved
@varCepheid
Copy link
Collaborator Author

The changes to the report and saved-report routes are because the logic on the back button used to discard changes to a report being edited, but with this PR in order to maintain that logic we needed new pages.

We Do Not Need These Extra Spaces --Caleb Eby

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

@calebeby calebeby calebeby requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@varCepheid varCepheid

Labels
enhancement Adds a new feature or meets a request. priority This needs to be addressed before the current changes can be merged to main.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

back button from match page
2 participants
@varCepheid @calebeby