Skip to content

Conversation

@lidiazuin
Copy link
Collaborator

No description provided.

@lidiazuin lidiazuin requested a review from recrwplay November 1, 2024 14:20
@netlify
Copy link

netlify bot commented Nov 1, 2024

Deploy Preview for neo4j-docs-ui ready!

Name Link
🔨 Latest commit 22aa906
🔍 Latest deploy log https://app.netlify.com/sites/neo4j-docs-ui/deploys/674ec8baf6e1a50008fb2540
😎 Deploy Preview https://deploy-preview-278--neo4j-docs-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

src/css/doc.css Outdated
.doc a.external:hover::after {
top: -12px;
opacity: 0;
text-decoration: underline;
Copy link
Collaborator

@recrwplay recrwplay Nov 4, 2024

Choose a reason for hiding this comment

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

I would remove this. Links are already underlined, so adding this text-decoration just adds an underline to the arrow when hovering over an external link. (This change applies to all external links in the docs).

(I'm happy to make the other changes to external links, though.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is because for some reason the arrow to the external link is disappearing when hovering

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I don't really like that animation. The two lines that are deleted here I'm ok with that change. I just wouldn't add the text-decoration here.

src/css/doc.css Outdated
.doc .olist li,
.doc .ulist li {
margin-bottom: 0.5rem;
margin-bottom: 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will change the appearance of all unordered lists in the docs. If we want to update the lists in the home page, or just the 'Keep exploring' card, we should create a more specific rule that applies to those link - a rule for something like body.docs-ndl .ulist li... or body.docs-ndl div.lists .ulist li... in docs-ndl.css

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the Keep exploring card is barely being used and the places where it still exists need to be updated, but if this affects the content in the doc pages in a disruptive way, then yes. Do you think that is the case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this would apply to any page generated from the UI bundle because we wrap the page contents in <article class="doc">...</article>

Copy link
Collaborator Author

@lidiazuin lidiazuin Nov 6, 2024

Choose a reason for hiding this comment

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

Do you think a 0.5rem difference could be disruptive to the rest of the docs? If so, then it's better to create a new rule like you suggested

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes list items more readable when the content wraps onto multiple lines. Without it, the spacing between the lines of a list item and the spacing between two list items are pretty much the same. The effect is similar to what would happen if there was no margin below a paragraph.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, so I will fix it as you suggested!

Copy link
Collaborator

@recrwplay recrwplay left a comment

Choose a reason for hiding this comment

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

Let's merge this

@lidiazuin lidiazuin merged commit 6f2d830 into neo4j-documentation:master Dec 4, 2024
4 of 5 checks passed
@lidiazuin lidiazuin deleted the homepage-fixes branch December 4, 2024 11:12
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.

2 participants