sigit/sigit.io
6
2
Fork 0
Code Issues 41 Pull Requests 2 Actions Packages Projects 1 Releases Wiki Activity

Add Sigit ID as a path param #195

Open
m wants to merge 10 commits from issue-171 into staging
pull from: issue-171
merge into: sigit:staging
Conversation 2 Commits 10 Files Changed 4 +52 -19
m commented 2024-09-10 14:01:34 +00:00
Owner
Copy Link

Closes #171

Intent

When opening a /sign page, we do not link a particular sigit which we are opening, and we pass the info about sigit in the location.state object.

To make it linkable, I've added an optional :id param in the /sign route and stopped providing state in the location.state.

id param is actually the Sigit ID which is the create event id.

Closes #171 ## Intent When opening a `/sign` page, we do not link a particular `sigit` which we are opening, and we pass the info about `sigit` in the `location.state` object. To make it linkable, I've added an optional `:id` param in the `/sign` route and stopped providing state in the `location.state`. `id` param is actually the `Sigit ID` which is the `create event id`.
m added 1 commit 2024-09-10 14:01:34 +00:00
feat: Add Sigit ID as a path param
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 33s
Details
75a715d002
enes requested changes 2024-09-10 15:53:54 +00:00
Dismissed
src/pages/sign/index.tsx Outdated
@ -73,3 +75,1 @@
arrayBuffer: decryptedArrayBuffer,
uploadedZip
} = location.state || {}
const [metaInNavState, setMetaInNavState] = useState<Meta | undefined>()
enes commented 2024-09-10 15:49:34 +00:00
Member
Copy Link

We should just replace the value of metaInNavState without having states.
We can just move it out of the destructuring and modify it if needed? Something like?

  const { arrayBuffer: decryptedArrayBuffer, uploadedZip } = location.state

  let metaInNavState = location.state.meta
  if (usersAppData) {
    const sigitId = params.id

    if (sigitId) {
      metaInNavState = usersAppData.sigits[sigitId]
    }
  }
We should just replace the value of `metaInNavState` without having states. We can just move it out of the destructuring and modify it if needed? Something like? ``` const { arrayBuffer: decryptedArrayBuffer, uploadedZip } = location.state let metaInNavState = location.state.meta if (usersAppData) { const sigitId = params.id if (sigitId) { metaInNavState = usersAppData.sigits[sigitId] } } ```
m marked this conversation as resolved
src/routes/index.tsx
@ -19,6 +19,7 @@ export const appPrivateRoutes = {
homePage: '/',
create: '/create',
sign: '/sign',
enes commented 2024-09-10 15:06:03 +00:00
Member
Copy Link

Can we use optional segment? Instead of two routes /sign and /sign/:id we can combine it into /sign/:id? .

Can we use optional segment? Instead of two routes `/sign` and `/sign/:id` we can combine it into `/sign/:id?` .
m marked this conversation as resolved
eugene commented 2024-09-11 09:34:49 +00:00
Owner
Copy Link

Can you please add update the description of the PR with its primarily objective and what changes have been made?

Can you please add update the description of the PR with its primarily objective and what changes have been made?
m added 2 commits 2024-09-11 10:12:43 +00:00
style: lint fix
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 34s
Details
7c027825cd
b commented 2024-09-11 10:15:37 +00:00
Owner
Copy Link

I would like to see:

  • more comments (eg, explanation of what the id is)
  • consistent, clear naming convention. sigit, sigitid, id, sigitkey (what are these?) vs sigitCreateId -> selfExplanatory
I would like to see: - more comments (eg, explanation of what the id is) - consistent, clear naming convention. sigit, sigitid, id, sigitkey (what are these?) vs `sigitCreateId` -> selfExplanatory
👍 1
m added 1 commit 2024-09-11 10:29:48 +00:00
chore: comments and lint (typing)
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 33s
Details
86a16c13ce
m added 1 commit 2024-09-11 10:33:48 +00:00
chore: sigitCreateId naming
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 33s
Details
5dc8d53503
enes approved these changes 2024-09-11 11:09:17 +00:00
b reviewed 2024-09-11 11:18:37 +00:00
src/pages/sign/index.tsx Outdated
@ -76,0 +81,4 @@
}
/**
* If userAppData (redux) is available, and we have route param (sigit id)
b commented 2024-09-11 11:18:37 +00:00
Owner
Copy Link

sigit id - what is this? should be defined

sigit id - what is this? should be defined
m marked this conversation as resolved
m added 1 commit 2024-09-11 11:30:51 +00:00
chore: renamed sigitKey to sigitCreateId
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 34s
Details
64e8ebba85
m added 1 commit 2024-09-11 13:41:50 +00:00
chore: comment fix
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 34s
Details
79e14d45a1
m added 1 commit 2024-09-11 15:29:55 +00:00
fix: verify/sign link
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 34s
Details
e48a396990
b added 1 commit 2024-09-13 10:07:27 +00:00
Merge branch 'staging' into issue-171
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 34s
Details
aa8214d015
enes added 1 commit 2024-09-16 12:37:42 +00:00
Merge branch 'staging' into issue-171
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 34s
Details
6ba3b6ec89
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 34s
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin issue-171:issue-171
git checkout issue-171
Sign in to join this conversation.
Reviewers
No reviewers
enes
Labels
Clear labels   
Kind/Breaking

Breaking change that won't be backward compatible

  
Kind/Bug

Something is not working

  
Kind/Documentation

Documentation changes

  
Kind/Enhancement

Improve existing functionality

  
Kind/Feature

New functionality

  
Kind/Security

This is security issue

  
Kind/Testing

Issue or pull request related to testing

  
Priority
Critical
The priority is critical

  
Priority
High
The priority is high

  
Priority
Low
The priority is low

  
Priority
Medium
The priority is medium

  
Reviewed
Confirmed
Issue has been confirmed

  
Reviewed
Duplicate
This issue or pull request already exists

  
Reviewed
Invalid
Invalid issue

  
Reviewed
Won't Fix
This issue won't be fixed

  
Status
Abandoned
Somebody has started to work on this but abandoned work

  
Status
Blocked
Something is blocking this issue or pull request

  
Status
Need More Info
Feedback is required to reproduce issue or to continue work

No Label
Kind/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: sigit/sigit.io#195
No description provided.
Delete Branch "issue-171"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?