Skip to content

Improve submit achievements UI by adding a Preview Feature#464

Open
KanishkaSingh-18 wants to merge 2 commits into
meswaramuthu:mainfrom
KanishkaSingh-18:first-contribution
Open

Improve submit achievements UI by adding a Preview Feature#464
KanishkaSingh-18 wants to merge 2 commits into
meswaramuthu:mainfrom
KanishkaSingh-18:first-contribution

Conversation

@KanishkaSingh-18

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

Users didn't have to preview the achievement the details before submitting. This can lead to mistakes or multiple submissions. Adding a preview step can help users confirm everything is correct before the final submission, improving the overall experience.

What changes are included in this PR?

  • Adding a preview model that shows all achievement details before final submission
  • Added a "Preview" button to open the model
  • Updated UI styling for better readability in the model
  • No backend logic changes

Are these changes tested?

Yes, I tested locally.
Checked the preview modal with different inputs, verified it shows the correct details and confirmed the final submission works as expected.

Are there any user-facing changes?

Yes, there is a new "preview" step before submitting achievements. Users will now see a summary of their entry in a modal and can confirm or go back to edit before submitting. This makes the process clearer and reduces accidental mistakes

@vercel

vercel Bot commented May 23, 2026

Copy link
Copy Markdown

@KanishkaSingh-18 is attempting to deploy a commit to the 007's projects Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions

Copy link
Copy Markdown

Thanks for creating a PR for your Issue! ☺️

We'll review it as soon as possible.
In the meantime, please double-check the file changes and ensure that all commits are accurate.

If there are any unresolved review comments, feel free to resolve them. 🙌🏼

@KanishkaSingh-18

Copy link
Copy Markdown
Author

Hey @Eswaramuthu
I’ve fixed the issue and pushed the changes. Only Vercel deployment authorization is pending from the team side, could you please approve it?

Thanks!

Comment thread templates/submit_achievements.html Outdated
Comment on lines +73 to +75
position: fixed;
inset: 0;
display: flex;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MAJOR BUG CSS .modal { display:flex } overrides hidden attribute, making modal always visible

Author stylesheets take precedence over the browser UA stylesheet's [hidden] { display:none }. So .modal { display:flex } wins and the overlay is rendered on every page load, not just when opened.

Suggested change
position: fixed;
inset: 0;
display: flex;
.modal:not([hidden]) {
position: fixed;
inset: 0;
display: flex;
align-items: center;
justify-content: center;
background: rgba(0,0,0,0.4);
padding: 1rem;
z-index: 9999;
}
Prompt to fix with AI

Copy this prompt into your AI coding assistant to fix this issue.

In `templates/submit_achievements.html`, lines 72–81, the CSS rule `.modal { display: flex; ... }` overrides the browser's built-in `[hidden] { display: none }` rule, causing the modal overlay to appear on every page load instead of only when opened. Fix: change the selector from `.modal` to `.modal:not([hidden])` so the flex layout only applies when the hidden attribute is absent. Keep all other declarations the same.

Comment thread templates/submit_achievements.html Outdated
Comment on lines +115 to +116
var studentInput = form.querySelector('input[name="student_id"]');
var fileInput = form.querySelector('input[type="file"][name="certificate"]');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MAJOR BUG Null dereference on form.querySelector when success page renders (no form in DOM)

When {% if success %} is true the form is not rendered, so both querySelector calls return null. form.querySelector(...) on line 115 then throws TypeError: Cannot read properties of null, crashing the entire script on the success page.

Suggested change
var studentInput = form.querySelector('input[name="student_id"]');
var fileInput = form.querySelector('input[type="file"][name="certificate"]');
var studentInput = form ? form.querySelector('input[name="student_id"]') : null;
var fileInput = form ? form.querySelector('input[type="file"][name="certificate"]') : null;
Prompt to fix with AI

Copy this prompt into your AI coding assistant to fix this issue.

In `templates/submit_achievements.html`, lines 115–116, `form.querySelector(...)` is called without checking whether `form` is null. When the success message is displayed, no `<form>` exists in the DOM, so `form` is null and this throws a TypeError. Fix: add a null guard — change lines 115–116 to `var studentInput = form ? form.querySelector('input[name="student_id"]') : null;` and `var fileInput = form ? form.querySelector('input[type="file"][name="certificate"]') : null;`.

@entelligence-ai-pr-reviews

Copy link
Copy Markdown
Contributor

Confidence Score: 2/5 - Changes Needed

Not safe to merge — this PR introduces a preview modal feature in submit_achievements.html but contains two functional bugs that will break the UI. The CSS rule .modal { display: flex } overrides the browser's [hidden] attribute handling, causing the modal to be permanently visible instead of hidden by default, which breaks the core show/hide behavior of the preview feature. Additionally, the JavaScript form.querySelector calls will throw a null dereference error on the success page because the {% if success %} branch omits the form element from the DOM entirely.

Key Findings:

  • .modal { display: flex } in the inline stylesheet takes author-stylesheet precedence over the UA stylesheet's [hidden] { display: none }, so the modal renders as always-visible regardless of the hidden attribute — the toggle logic is effectively broken from page load.
  • When {% if success %} is truthy, the form is not rendered, yet the JavaScript still calls form.querySelector(...) unconditionally, resulting in a null dereference TypeError that will crash the script and potentially break the success page entirely.
  • The PR's goal of adding a preview feature is reasonable and the modal structure itself is sensible, but both bugs are in the critical path of the feature's core behavior (show/hide and form data extraction), meaning the feature does not work correctly as submitted.
Files requiring special attention
  • templates/submit_achievements.html

@entelligence-ai-pr-reviews

Copy link
Copy Markdown
Contributor

EntelligenceAI PR Summary

Refactored templates/submit_achievements.html with structural, style, and JavaScript improvements.

  • Fixed spacing in Jinja2 url_for calls
  • Corrected indentation and alignment of {% if success %}/{% else %}/{% endif %} template blocks
  • Relocated <style> and <script> blocks to inside <body> before </body>
  • Updated modal CSS selector from .modal to .modal:not([hidden]) to avoid layout issues when hidden
  • Replaced var declarations and manual null-guards with const/let and optional chaining (?.)

Confidence Score: 5/5 - Safe to Merge

Safe to merge — this PR makes well-scoped, incremental improvements to templates/submit_achievements.html with no introduced logic bugs or security concerns. The changes modernize the JavaScript by replacing var with const/let, fix the .modal CSS selector to .modal:not([hidden]) to prevent unintended layout side effects when the modal is hidden, and correct structural issues like misplaced <style>/<script> blocks and Jinja2 template indentation. No review comments were generated, all changed files were reviewed, and the heuristic analysis surfaced zero issues at any severity level.

Key Findings:

  • The .modal:not([hidden]) CSS selector fix correctly scopes modal styles to only the visible state, preventing potential layout interference when the modal element carries a hidden attribute — a genuine correctness improvement with zero risk of regression.
  • Replacing var declarations with const/let and removing manual null-guards improves scoping correctness and eliminates a class of subtle bugs related to hoisting and accidental global variable leakage in the submit-achievements JavaScript.
  • Moving <style> and <script> blocks inside <body> before </body> is a structural improvement that ensures scripts have access to DOM elements without requiring explicit DOMContentLoaded guards, and is consistent with modern HTML best practices.
  • All changes are confined to a single template file (templates/submit_achievements.html), the scope is narrow and well-understood, and no backend logic, route handlers, or data processing paths are touched.
Files requiring special attention
  • templates/submit_achievements.html

@KanishkaSingh-18

Copy link
Copy Markdown
Author

Hi @Eswaramuthu ,

I hope you're doing well. I wanted to follow up on this PR. I addressed the requested changes and updated the PR around 3 weeks ago. Could you please review it when you have time?

Please let me know if any further modifications are needed from my side. Thank you for your time and effort in maintaining the project!

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.

1 participant