Conversation
Future Improvement SuggestionConsider creating a reusable navigation button component The "Go to Project Management" button is now implemented across multiple interfaces with similar CSS and JavaScript patterns:
A shared
This isn't blocking for this PR, but worth discussing with the team for a future refactor. We might have something in /components/gui we can use. It is a little messy switching between using it in /components and using it on /interfaces it would be nice to use a consistent mechanic here. |
📋 Technical Debt Note: CheckPermissions Dual ImplementationDuring review, I noticed the codebase has two CheckPermissions implementations with identical method names but different return types:
Current State
All current usages are correct - the issue is naming confusion since both export Why This Matters
Recommended Future ActionConsider renaming exports for clarity in a future PR:
This would prevent future confusion without changing functionality. This note is for team awareness - no action needed for this PR. |
|
I triggered the preview deploy action and I think https://6972de70dccab7f7f1d1d861--tpen-interfaces-preview.netlify.app/ includes all the code changes from review. |
Clicking the link in the header loads to this error. The same error shows if I click "Parse Lines" from the management interface. The same project at https://app.t-pen.org/annotator?projectID=6967a514f7caf0d95c4252fb&pageID=6967a51bf7caf0d95c4252fd loads without error. |
But not on localhost. The dev preview deploy is not deploying the latest code. It seems to only deploy a snapshot of what the code was when the PR was created. |
| <a class="nav-icon" href="/index"><img draggable="false" src="../../assets/icons/home.png" alt="Home"></a> | ||
| <a class="nav-icon" href="/project/manage?projectID=${TPEN.screen.projectInQuery}"><img draggable="false" src="../../assets/icons/contact.png" alt="Manage Project"></a> | ||
| <a class="nav-icon" href="/profile"><img draggable="false" src="../../assets/icons/profile.png" alt="Profile"></a> | ||
| <a title="Home" class="nav-icon" href="/index"><img draggable="false" src="../../assets/icons/home.png" alt="Home"></a> |
There was a problem hiding this comment.
I don't think "Identify Lines" belongs here. This space is for Project Header level actions.
- Going home to switch projects or start over
- Logging out to 'shut down' work
- (peer-level concern) profile controls
- Manage this project
Previously, lines were in the Page Tools pane and I don't see a good reason to change that.
| <style> | ||
| #projectManagementBtn { | ||
| position: absolute; | ||
| top: 4.85em; | ||
| display: none; | ||
| background-color: var(--primary-color); | ||
| padding: 10px 20px; | ||
| color: var(--white); | ||
| border-radius: 5px; | ||
| cursor: pointer; | ||
| transition: background-color 0.3s; | ||
| } | ||
|
|
||
| #projectManagementBtn:hover { | ||
| background-color: var(--primary-light); | ||
| } | ||
|
|
||
| #projectManagementBtn span { | ||
| position: relative; | ||
| left: -10px; | ||
| display: inline-block; | ||
| transform: rotate(180deg); | ||
| } | ||
| </style> |
There was a problem hiding this comment.
I agree with this. Maybe @copilot can ticket it as an issue.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <style> | ||
| #projectManagementBtn { | ||
| position: absolute; | ||
| top: 5.1em; | ||
| display: none; | ||
| background-color: var(--primary-color); | ||
| padding: 10px 20px; | ||
| color: var(--white); | ||
| border-radius: 5px; | ||
| cursor: pointer; | ||
| transition: background-color 0.3s; | ||
| border: none; | ||
| } | ||
|
|
||
| #projectManagementBtn:hover, | ||
| #projectManagementBtn:focus-visible { | ||
| background-color: var(--primary-light); | ||
| outline: 2px solid var(--primary-color); | ||
| outline-offset: 2px; | ||
| } | ||
|
|
||
| #projectManagementBtn span { | ||
| position: relative; | ||
| left: -10px; | ||
| display: inline-block; | ||
| transform: rotate(180deg); | ||
| } | ||
| </style> |
There was a problem hiding this comment.
The #projectManagementBtn styling block is duplicated (with near-identical rules) in multiple places (interfaces/quicktype/index.html, interfaces/project/options.html, interfaces/manage-project/collaborators.html, and components/manage-role/index.js). Consider extracting this into a shared CSS class (e.g. .project-management-btn) in a common stylesheet or component-level style module and using class="project-management-btn" instead, to reduce duplication and keep future visual changes consistent.
cubap
left a comment
There was a problem hiding this comment.
YES! I love it and find no fault. There are good issues to move forward with.
Closes #389.
Ignores #391, #392 , and #393
Changes and scope notes below.
Login and Logout
Home
Transcription Workflow
Project Management Workflow
About Us & Contact Us
Shower Thoughts
These are for later.