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

CSS fix on links in sidebar #604

Merged
merged 5 commits into from
Sep 19, 2020

Conversation

johnkuney
Copy link
Contributor

No description provided.

@deadalnix
Copy link
Contributor

What problem does this solve?

@johnkuney
Copy link
Contributor Author

Sorry should have left a description. Just a style patch for the links that are in the sidebar component in the getting started pages (/buy-bitcoin-cash/ ex) The text links within the paragraph needed to be specified to inline so they dont break lines

Copy link
Collaborator

@jasonbcox jasonbcox left a comment

Choose a reason for hiding this comment

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

I'm not sure why forcing them to not break is beneficial. I now see something like this:

Some text...
<long-link>
.

Note the period on a line all by itself.

@johnkuney
Copy link
Contributor Author

I said the wrong word. I want the links to break. Currently they take an entire line of the paragraph like you are describing above. By making them inline they should flow with the paragraph right? I'm seeing the opposite of what youre describing with this change. Are you seeing that running this or thats what you see on the live site?

@deadalnix
Copy link
Contributor

Ok so your problem is that the sidebar display are defined as flex, which is good, but every child is also defined as flex, which is definitely wrong. You need to fix the sidebar CSS instead of doing this.

@deadalnix
Copy link
Contributor

You want to change this into .card > a or something like that.

specified a class for the section links so flex is not applied to all <a>s
@johnkuney
Copy link
Contributor Author

Okay I specified a class for the links that need flex so it doesnt get applied to the others

jasonbcox
jasonbcox previously approved these changes Sep 17, 2020
@deadalnix
Copy link
Contributor

You want to change this into .card > a or something like that.

Copy link
Contributor

@deadalnix deadalnix left a comment

Choose a reason for hiding this comment

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

You don't need to add a new class or anything. See comment I made twice now.

@johnkuney
Copy link
Contributor Author

this doesnt work for the rows that need to be links on the wallet page

@johnkuney
Copy link
Contributor Author

those are a children too and need specific style

Copy link
Contributor

@deadalnix deadalnix left a comment

Choose a reason for hiding this comment

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

I don't think we have clickableRow anywhere in the codebase at this time, so i don't see how that CSS element makes any sense.

display: flex;
color: #444;
transition: all 300ms;
}

.card a:hover {
.clickableRow:hover {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this clickableRow now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats the class name

@deadalnix
Copy link
Contributor

deadalnix commented Sep 18, 2020

This is wrong: https://github.com/bitcoincashorg/bitcoincash.org/pull/604/files#diff-c084369006e972f978373026b3dc660cR14

It defines all the childs as flex too.

@johnkuney
Copy link
Contributor Author

yes thats why I added the class before. Some of the links in the sidebar need different styling

@deadalnix
Copy link
Contributor

There are no other links in your patch that use the class, so I still don't see how any of this makes any sense.

@johnkuney
Copy link
Contributor Author

There are two possible types of links that need different styling in this sidebar component.

One is the type that I made this class for. It needs display: flex some colors etc. These are the anchor links that take you to different sections found on the wallet page for example that take up a whole row.

The other type is a link within the paragraph text. These only need their color defined and the rest of the default styling is fine. So I selected them through their parent class and applied the color no class needed

@deadalnix
Copy link
Contributor

The anchor links are not immediate childs of .card are they?

The these anchor link cannot appear anywhere else. So why .card > a inadequate?

@johnkuney johnkuney dismissed a stale review via 6238d6e September 18, 2020 23:50
@johnkuney
Copy link
Contributor Author

The anchor links are not direct children of .card. They are children of .cardIconRow so .cardIconRow > a works

@deadalnix deadalnix merged commit 5f92c74 into bitcoincashorg:master Sep 19, 2020
EyeOfPython pushed a commit to EyeOfPython/bitcoincash.org that referenced this pull request Nov 17, 2020
* css fix on links

* CSS cleanup

specified a class for the section links so flex is not applied to all <a>s

* remove class

* add a class

* remove class use child selector
@shadi19838410 shadi19838410 linked an issue Mar 23, 2023 that may be closed by this pull request
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.

RAY Tropics ( MD SHADI )
3 participants