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

Fixes for PR 57 #58

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fixes for PR 57 #58

wants to merge 6 commits into from

Conversation

davidbruce
Copy link

@marc0der Here is a fix for floating $ on certain inline text from #57. I created a new class called mono-highlight and replaced the inline code tags with mono-highlight span tags

Let me know if there are any other issues.

David Bruce and others added 3 commits November 26, 2021 23:45
This allows users to copy text without copying the $.
Replaces certain code tags  that do not need to show a $ with a mono-highlight span tag.
Changes New Open Source section wording.
@marc0der
Copy link
Member

marc0der commented Dec 5, 2021

I wondered if it wouldn't be better to have a class called command that would prefix the $ wherever needed. Once that's done, we could use the more generic code class for all standard code blocks. That would make the code a lot more understandable. wdyt?

@davidbruce
Copy link
Author

The areas where code was being used to highlight things weren't really code blocks though. The areas that I wrapped in span were mostly highlighting variable names, keywords, or file names such as:

* <span class="mono-highlight">.sdkmanrc</span>
* <span class="mono-highlight">java_home</span>
* <span class="mono-highlight">sdkReleaseVersion</span>

I think keeping the $ for <code> by default is still the way to go since it is the most common use case on the site.
All of the grey blocks we see are from the <pre> tag so the nested <code> tag is used to add the $. Then other locations that show inline highlighted commands can still be just wrapped in a <code> tag.

Instead of using span with a class I could use the <var> tag.

@marc0der
Copy link
Member

marc0der commented Dec 6, 2021

I've pulled down your branch and run it locally and still see lots of dollars all over where they don't belong. I've taken care to clear my browser cache but that wasn't it either. Also, when I try to use the span tags with the class="mono-highlight", the font changes to a sans font. The branch also has conflicts that are proving gnarly to resolve.

For now, I think I'm going to roll back the changes as things are getting too hairy for me. If you feel up to it, I suggest raising a new PR with all your changes together in one go.

I'm sorry about that as I know you've already spent substantial time on this.

Fixes Install page floating $ by replacing certain code tags with var.
Fixes Home page floating $ by replacing certain code tags with var.
@davidbruce
Copy link
Author

@marc0der I'm really sorry about all the hassle. I went ahead with the <var> over <span> change and I made sure all the CSS is correct. I also found the bad $s you mentioned and those should be all taken care of now(famous last words). I did purposely add $ to the curl blocks in the vendor section since it seems fitting. Lastly, all of the merge conflicts have been taken care of.

@marc0der
Copy link
Member

marc0der commented Dec 7, 2021

No worries, I'll take another look over the weekend when I have some time.

@davidbruce
Copy link
Author

Looks like there are some conflicts with the most recent changes, I'll get them sorted tomorrow morning.

@marc0der
Copy link
Member

marc0der commented Feb 12, 2022

Hi @davidbruce,

As you probably realise, I had to revert #57 because it broke our site. To add to this, #58 continues adding more stuff that is not strictly related to the one thing you set out to do, which was placing focus on Getting started. Here is what I suggest:

I love all your ideas about the following changes you wanted to bring about:

  • put focus on the Getting started section
  • floating $ everywhere to facilitate copy and paste
  • removal of hardcoded <code> tags

I would hate for us to miss out on your great ideas, so I would like to ask if you could consider raising three small PRs for these individual points. Having them separate makes it easier for us to review these changes and causes fewer problems with merging. Also, could I please ask you not to merge from master into your feature branch but rather to rebase against master before raising each PR? This results in a cleaner timeline in Git and makes things easier to maintain. Finally, because of the state of these branches, I suggest just doing these bits over and letting us review them one by one.

Hope you don't mind and that you're up for this! 😄

@davidbruce
Copy link
Author

Yeah I can split these into some PRs next week for ya. Sorry I let this fall by the wayside.

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.

None yet

2 participants