Resolver and Rapid resolver stable Id redirect Intermediate page #15

shashankbrgowda · 2025-09-10T16:00:25Z

This PR introduces an intermediate redirect page for Resolver and Rapid Resolver stable ID redirects.

Details

Added rapid stable Id /rapid/id/{stable_id} redirect endpoint similar to existing resolver stable id redirect endpoint
Opens an Intermediate page for rapid stable Id redirect
For resolver stable Id redirect endpoint, If there are multiple results from metadata API then return an Intermediate page

app/api/resources/templates/resolver/resolver_id_content.html

azangru · 2025-09-11T21:10:18Z

app/api/resources/templates/resolver/resolver_id.html

+    <link rel="preconnect" href="https://fonts.googleapis.com">
+    <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
+    <link href="https://fonts.googleapis.com/css2?family=Lato:wght@300;400;700&display=swap" rel="stylesheet">
+    <link rel="stylesheet" type="text/css" href="/static/css/rapid_page.css">


Is this a template page for non-rapid urls? Why does it have the rapid css link?

azangru · 2025-09-11T21:11:23Z

app/api/resources/templates/resolver/resolver_id.html

@@ -0,0 +1,23 @@
+<!doctype html>


These two html pages have very similar heads. Would it make sense to extract the common parts into reusable partials?

azangru · 2025-09-11T21:16:16Z

app/api/resources/templates/shared/id_content.html

+                        {% elif item.type %}
+                          {{ item.type.kind }} - {{ item.type.value }},
+                        {% endif %}
+                    </span>


http://0.0.0.0:8001/rapid/id/LOC122384268

We probably don't want this empty space if there's no content for this block.

app/api/resources/templates/shared/id_content.html

azangru · 2025-09-11T21:59:29Z

Another thing that I always end up complaining about is that I don't understand how the pages are supposed to work on different screen sizes. Sure; we don't expect most users to have small screens (although you don't need to have a small screen to have a small viewport); but what is the page supposed to do if that happens? Is it supposed to look this way?

responsive.mp4

P.S.: consider that on small screens, the tooltip will be in danger of doing this (yet another reason I am against this idea):

azangru · 2025-09-11T22:05:08Z

app/api/utils/commons.py

@@ -0,0 +1,35 @@
+from typing import List


In a comment to a PR in amr-portal, @veidenberg was saying that list no longer needs to be imported from typing.

azangru · 2025-09-11T22:07:08Z

app/api/resources/templates/shared/id_content.html

+    </div>
+</div>
+
+<script>


I wonder: if css is served as a separate file, would it be worth moving js into its own file as well?

azangru · 2025-09-11T22:10:03Z

app/api/resources/templates/shared/id_content.html

+                    data-entity-viewer-url="{{ item.entity_viewer_url }}">
+                    <span class="id-detail">{{ item.common_name }},</span>
+                    <span class="id-detail id-detail-margin-left id-detail-italic">{{ item.scientific_name }},</span>
+                    <span class="id-detail id-detail-margin-left id-detail-italic">


Type isn't italic in the design (see XD). Same as on the beta site.

azangru · 2025-09-11T22:12:15Z

app/api/resources/templates/shared/id_content.html

+                        {% endif %}
+                    </span>
+                    <span class="id-detail id-detail-margin-left">{{ item.assembly.name }},</span>
+                    <span class="id-detail id-detail-margin-left id-detail-tooltip">{{ item.assembly.accession_id }}</span>


There is something going on with font sizes in the design. I can't tell what exactly, but something definitely is up.

azangru · 2025-09-11T22:16:45Z

app/static/css/rapid_page.css

+
+.tooltip-content {
+  background: #fff;
+  border: 1px solid #ccc;


Why? Is there a reason for this in the design?

azangru · 2025-09-11T22:18:01Z

app/static/css/rapid_page.css

+  padding: 12px;
+  border-radius: 1px;
+  position: relative;
+  background-color: #1b2c39;


You may want to move this into a css custom property (on beta site, it is --color-black)

azangru · 2025-09-11T22:20:38Z

app/static/css/rapid_page.css

+.id-detail {
+  font-size: 12px;
+  cursor: pointer;
+  color: var(--color-light-blue);


Where did you get the value for this colour from? Isn't Andrea reusing the colour palette from the beta site?

From ensembl-client https://github.com/Ensembl/ensembl-client/blob/main/src/styles/design-tokens.css#L12

azangru · 2025-09-11T22:24:01Z

app/static/css/rapid_page.css

+  margin-top: 15px;
+}
+
+.id-detail {


I would rename those id-detail classes. The elements on the page that have this class do not represent details of some id. They represent species, or genomes, or search matches, but not ids.

azangru · 2025-09-11T22:24:47Z

app/static/css/rapid_page.css

+}
+
+.id-detail {
+  font-size: 12px;


Are you sure about the font size here? Compare with beta.

Checked. Its either 12px, 13px or 16px..

azangru · 2025-09-11T22:25:18Z

app/static/css/rapid_page.css

+
+.id-detail {
+  font-size: 12px;
+  cursor: pointer;


You wouldn't need this if the element were a link.

azangru · 2025-09-11T22:26:48Z

app/api/resources/templates/shared/id_content.html

+<script>
+    document.querySelectorAll('.id-detail-container').forEach(function(el) {
+      el.addEventListener('click', function(e) {
+        e.stopPropagation();


azangru · 2025-09-11T22:28:57Z

app/api/resources/templates/shared/id_content.html

+
+        if (!e.target.classList.contains('id-detail')) return;
+
+        var tooltip = document.getElementById('tooltip');


The front-end world today has largely rejected vars. The overall consensus is to use either let or const

azangru · 2025-09-11T22:31:42Z

app/api/resources/templates/shared/id_content.html

+
+    window.addEventListener('resize', function() {
+      var tooltip = document.getElementById('tooltip');
+      if (tooltip) {


why are you making this check in this function, but not in the first one?

Updated in all places

veidenberg · 2025-09-12T08:35:43Z

app/api/models/resolver.py

@@ -1,6 +1,6 @@
 from enum import Enum
 from typing import Optional, Literal, List, Dict, Annotated


No need to import Optional, List, Dict (builtin types) and Annotated (not used in code)

veidenberg · 2025-09-12T08:36:17Z

app/api/models/resolver.py

-    assembly: Assembly
-    scientific_name: str
-    common_name: str
+    assembly: Optional[Assembly] = None


assembly: Assembly | None = None recommended, same for all Optionals below

Nice; python is looking more similar to typescript 🙂

veidenberg · 2025-09-12T09:54:41Z

app/api/models/resolver.py

+    message: Optional[str] = None
+    rapid_archive_url: Optional[str] = None

    class Config:


Config class is deprecated, use ConfigDict instance instead (seems you even added ConfigDict import but stopped there :)

Suggested change

class Config:

model_config = ConfigDict(use_enum_values = True)

veidenberg · 2025-09-12T10:04:41Z

app/api/models/resolver.py

+    code: Optional[int] = None
+    message: Optional[str] = None
+    rapid_archive_url: Optional[str] = None
+    content: Optional[List[StableIdResolverContent]] = []


mutable default value (list) is not recommended as it can share state across the response instances. Use pydantic default_factory, or better yet, None (since it's a valid value here):

Suggested change

content: Optional[List[StableIdResolverContent]] = []

content: list[StableIdResolverContent] | None = None

veidenberg · 2025-09-12T10:25:17Z

app/api/resources/rapid_view.py

+            content=None,
+            rapid_archive_url=rapid_archive_url
+        )
+        return HTMLResponse(generate_rapid_id_page(res))


did you mean to pass a model or dict to generate_rapid_id_page()? For dict use res.model_dump() (like you do on line 58 below).

Not important for html response.. Changed to return object

For Json response its important to ignore certain fields

veidenberg

Added few minor comments

shashankbrgowda · 2025-09-12T13:08:35Z

Thanks @veidenberg @azangru

Updated the PR as per your suggestions

veidenberg

Looks good to me (even with some Optionals still left behind:)

azangru · 2025-09-15T07:29:10Z

app/api/resources/templates/shared/id_content.html

+                <div class="genome-detail-container"
+                    data-genome-browser-url="{{ item.genome_browser_url }}"
+                    data-entity-viewer-url="{{ item.entity_viewer_url }}">
+                    <a href="{{ item.entity_viewer_url }}" class="genome-detail genome-detail-tooltip" aria-label="Show tooltip">


Why this aria-label? Aria-label is something that the screen reader will speak out loud. Normally, for links, the screen reader would just speak the content of the link. Surely this is more useful than if it says "Show tooltip".

azangru · 2025-09-15T07:34:35Z

app/api/resources/templates/shared/appbar.html

 <div class="topbar">
    <div class="topbar-left">
        <div class="home-link">
            <a href="https://beta.ensembl.org/">


Now this link is where aria-label is appropriate, because it doesn't have any text content. Could you add aria-label="Ensembl home page" to it?

azangru · 2025-09-15T07:38:53Z

app/api/resources/templates/shared/id_content.html

+            <div id="tooltip" style="display:none; position:absolute;" role="tooltip" aria-hidden="true">
+                <div class="tooltip-content">
+                    <span class="tooltip-content-title">View in</span>
+                    <a id="genome-browser-link" href="#" aria-label="Genome Browser link">


You do not need the word "link" in the aria-label. The screen reader already announces that this is a link.

Perhaps, to make the label slightly more descriptive, it could say "View in Genome Browser".

(Same for entity viewer)

azangru · 2025-09-15T07:42:05Z

app/static/css/styles.css

  --standard-gutter: 30px;
  --global-padding-left: calc(var(--standard-gutter) * 4);
-  --color-light-blue: #33adff;
+  --color-light-blue: #0099ff;


Could you please rename --color-light-blue to --color-blue, to keep consistency with colour names in beta?

azangru · 2025-09-15T07:50:55Z

app/static/css/styles.css

+.id-redirect-content {
+  margin-top: 30px;
+  text-align: center;
+  width: 100%;


Why is this CSS rule needed?

azangru · 2025-09-15T07:54:50Z

app/static/js/index.js

+  const tooltip = document.getElementById('tooltip');
+  if (tooltip) {
+    tooltip.style.display = 'none';
+    tooltip.setAttribute('aria-hidden', 'true');


Doesn't display: none already hide the tooltip from assistive technology?

azangru · 2025-09-15T07:56:25Z

app/static/js/index.js

+  const entityViewerLink = document.getElementById('entity-viewer-link');
+
+  if (genomeBrowserLink) {
+    genomeBrowserLink.href = el.dataset.genomeBrowserUrl || '#';


Does || # imply that the genomeBrowserUrl may not exist? If yes, then why show the link element at all?

azangru · 2025-09-15T07:59:17Z

app/static/js/index.js

+  tooltip.setAttribute('aria-hidden', 'false');
+  const rect = target.getBoundingClientRect();
+  tooltip.style.top = (rect.top - 15 + window.scrollY) + 'px';
+  tooltip.style.left = (rect.right + 10 + window.scrollX) + 'px';


Could you add an automatic focus on the first link? Something like:

const firstLink = tooltip.querySelector('a'); firstLink.focus();

shashankbrgowda added 2 commits September 10, 2025 16:59

resolver id and rapid id page

3a1aa71

fix

605d983

shashankbrgowda changed the title ~~resolver id and rapid id page~~ Resolver and Rapid resolver stable id redirect Intermediate page Sep 10, 2025

shashankbrgowda changed the title ~~Resolver and Rapid resolver stable id redirect Intermediate page~~ Resolver and Rapid resolver stable Id redirect Intermediate page Sep 10, 2025

azangru reviewed Sep 10, 2025

View reviewed changes

app/api/resources/templates/resolver/resolver_id_content.html Outdated Show resolved Hide resolved

azangru reviewed Sep 10, 2025

View reviewed changes

app/api/resources/templates/resolver/resolver_id_content.html Outdated Show resolved Hide resolved

shashankbrgowda added 2 commits September 11, 2025 08:50

reuse stable id page, fixes

472c614

fixes

466a59c

shashankbrgowda marked this pull request as ready for review September 11, 2025 08:10

shashankbrgowda requested a review from veidenberg September 11, 2025 08:10

azangru reviewed Sep 11, 2025

View reviewed changes

app/api/resources/templates/shared/id_content.html Show resolved Hide resolved

azangru reviewed Sep 11, 2025

View reviewed changes

app/api/resources/templates/shared/id_content.html Show resolved Hide resolved

azangru reviewed Sep 11, 2025

View reviewed changes

app/static/css/rapid_page.css Outdated

.id-detail {

font-size: 12px;

cursor: pointer;

Copy link

azangru Sep 11, 2025

Choose a reason for hiding this comment

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

You wouldn't need this if the element were a link.

azangru reviewed Sep 11, 2025

View reviewed changes

fixes

43e432a

update README

a40eb9a

veidenberg reviewed Sep 12, 2025

View reviewed changes

remove typing imports and genome detail fixes

2da6a71

shashankbrgowda added 3 commits September 12, 2025 14:23

add robots noindex follow meta tag

77b67b9

meta refresh tag fix

aeeccf7

invalid stable id page fix

05f4ded

veidenberg approved these changes Sep 12, 2025

View reviewed changes

azangru reviewed Sep 15, 2025

View reviewed changes

app/static/css/styles.css Outdated

.id-redirect-content {

margin-top: 30px;

text-align: center;

width: 100%;

Copy link

azangru Sep 15, 2025

Choose a reason for hiding this comment

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

Why is this CSS rule needed?

azangru reviewed Sep 15, 2025

View reviewed changes

review comment fixes

097ac71

azangru approved these changes Sep 15, 2025

View reviewed changes

shashankbrgowda merged commit 9f9af9a into main Sep 15, 2025
1 check passed


		if (!e.target.classList.contains('id-detail')) return;

		var tooltip = document.getElementById('tooltip');

		@@ -1,6 +1,6 @@
		from enum import Enum
		from typing import Optional, Literal, List, Dict, Annotated

	class Config:
	model_config = ConfigDict(use_enum_values = True)

	content: Optional[List[StableIdResolverContent]] = []
	content: list[StableIdResolverContent] \| None = None

Resolver and Rapid resolver stable Id redirect Intermediate page #15

Resolver and Rapid resolver stable Id redirect Intermediate page #15

Uh oh!

Conversation

shashankbrgowda commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azangru Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

azangru commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azangru Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veidenberg Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veidenberg Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veidenberg Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veidenberg Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veidenberg Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

shashankbrgowda commented Sep 10, 2025 •

edited

Loading

azangru Sep 11, 2025 •

edited

Loading

azangru commented Sep 11, 2025 •

edited

Loading

azangru Sep 11, 2025 •

edited

Loading

veidenberg Sep 12, 2025 •

edited

Loading

veidenberg Sep 12, 2025 •

edited

Loading

veidenberg Sep 12, 2025 •

edited

Loading

veidenberg Sep 12, 2025 •

edited

Loading

veidenberg Sep 12, 2025 •

edited

Loading

azangru Sep 15, 2025 •

edited

Loading