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.

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

[#1432] Opening links from FAQ #1633

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

linssen814
Copy link
Contributor

@linssen814 linssen814 commented Feb 21, 2025

Edited utils/ckeditor.py so that the links on the FAQ answers don't open in a new tab anymore. Also added link icons to them.

Taiga: https://taiga.maykinmedia.nl/project/open-inwoner/issue/1432

image

@linssen814 linssen814 force-pushed the issue/1432-links-openen-veelgestelde-vragen branch 3 times, most recently from c7b6bfe to e852417 Compare February 21, 2025 13:45
@linssen814 linssen814 requested review from vaszig and removed request for vaszig February 21, 2025 13:46
@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2025

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 94.18%. Comparing base (f76f455) to head (a1c4603).

Files with missing lines Patch % Lines
src/open_inwoner/utils/ckeditor.py 0.00% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1633      +/-   ##
===========================================
- Coverage    94.20%   94.18%   -0.02%     
===========================================
  Files         1086     1086              
  Lines        40026    40033       +7     
===========================================
  Hits         37705    37705              
- Misses        2321     2328       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@linssen814 linssen814 requested a review from vaszig February 21, 2025 14:26
@jiromaykin jiromaykin requested review from jiromaykin and removed request for vaszig February 24, 2025 08:29
Copy link
Contributor

@jiromaykin jiromaykin left a comment

Choose a reason for hiding this comment

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

Requesting changes:

  1. Change the underline/text-decoration. External links with an icon should only have their text itself be underlined, there should not be a line under the icon itself. For an example, look at the product pages, like this one https://open-inwoner-test.maykin.nl/onderwerpen/zorg-en-ondersteuning/producten/huiselijk-geweld/ that contains external links with proper icons and proper text-decoration:
  2. Slightly better process when creating a new PR: first switch back to Develop branch, then do a git pull, and then create a new branch from Develop. This way you will have a correct commit history. When a task takes longer you can just rebase to Develop, before doing your final commit and only then ask for review.
  3. Add a link to your PR in the Taiga issue as well.

@pi-sigma
Copy link
Contributor

@jiro @linssen814 Note that this PR is based on an older user story and seems to be in tension with the recent https://taiga.maykinmedia.nl/project/open-inwoner/us/3014 (PR: #1631), which requires that links to formulieren and acties be opened in a new tab. The rationale is that otherwise users get the wrong impression that closing the tab will destroy their OIP session (which is not the case because they will log in via an external service to create/resume a form, thus closing the tab will destroy that session but not their OIP session).

@linssen814 linssen814 force-pushed the issue/1432-links-openen-veelgestelde-vragen branch 5 times, most recently from bd93b2d to 88cb969 Compare February 25, 2025 09:50
@linssen814 linssen814 force-pushed the issue/1432-links-openen-veelgestelde-vragen branch from 88cb969 to a1c4603 Compare February 25, 2025 10:05
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.

4 participants