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

Opcode #135 / opcode #335 #130

Merged
merged 8 commits into from
May 18, 2022
Merged

Opcode #135 / opcode #335 #130

merged 8 commits into from
May 18, 2022

Conversation

4Luke4
Copy link
Contributor

@4Luke4 4Luke4 commented May 2, 2022

Added some rudimentary info about how to bypass the bug affecting these two opcodes.

@lynxlynxlynx
Copy link
Member

Please use the relurl function as discussed, so this will work properly also for @burner1024's projects.

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 3, 2022

Please use the relurl function as discussed, so this will work properly also for @burner1024's projects.

Isn't that only for yml files...?
I edited only standard html files this time...

@burner1024
Copy link
Contributor

To work in opcodes, relurl.html should also be included in op-list.html.

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 3, 2022

To work in opcodes, relurl.html should also be included in op-list.html.

Is everything OK, now?

Copy link
Contributor

@burner1024 burner1024 left a comment

Choose a reason for hiding this comment

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

Include before the for loop, otherwise it'll be included hundreds of times

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 3, 2022

Include before the for loop, otherwise it'll be included hundreds of times

Fixed.

@lynxlynxlynx
Copy link
Member

It's fine if @burner1024 doesn't care, otherwise just adding the include doesn't change anything.

@burner1024
Copy link
Contributor

burner1024 commented May 3, 2022

At the moment opcode descriptions are not being imported. But they likely will be sooner or later.

I think it makes sense to have a consistent policy for links, applying relurl everywhere.
Anyway, it's not something that's going to be done quickly, IESDP is too vast. Probably just fix them along the way while working on something else, like this pull?

@lynxlynxlynx
Copy link
Member

I agree, that's why I proposed to use it. @4Luke4 you can search for relurl to see how it's used as a liquid filter.

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 8, 2022

@4Luke4 you can search for relurl to see how it's used as a liquid filter.

Will do.
Feel free to merge then...

@lynxlynxlynx
Copy link
Member

Sorry, but the consensus above was to fix this now, not later, so we can get farther incrementally.

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 14, 2022

Sorry, but the consensus above was to fix this now, not later, so we can get farther incrementally.

@burner1024
OK, so what exactly...? Should I add {% include relurl.html %} to some other file...?

@burner1024
Copy link
Contributor

I think what @lynxlynxlynx means is that urls in this pull should be changed to follow the new scheme (with relurl filter.)

Urls now follow the new scheme (with `relurl` filter).
@4Luke4
Copy link
Contributor Author

4Luke4 commented May 16, 2022

I think what @lynxlynxlynx means is that urls in this pull should be changed to follow the new scheme (with relurl filter.)

Could you please check if everything is fine now...?

@burner1024
Copy link
Contributor

<a name="class" href="{{ '#op337' | prepend: relurl }}">opcode #337</a>

"class" is just an example, for when you do want to add a name to the link, and markdown doesn't support it, and then we use html intead. You don't need to add it to every link.
And there's no need to add relurl to anchors that lead to the same page. They are relative anyway:

<a href="#op337">opcode #337</a>

<a name="class" href="{{ '../file_formats/ie_formats/eff_v2.htm' | prepend: relurl }}">EFF</a>

All urls should start from root of the site. No ../ anywhere:

<a href="{{ '/file_formats/ie_formats/eff_v2.htm' | prepend: relurl }}">EFF</a>

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 17, 2022

All urls should start from root of the site. No ../ anywhere:

This leads to an error

`/file_formats/ie_formats/eff_v2.htm' not found.

@burner1024
Copy link
Contributor

burner1024 commented May 17, 2022

Where? Push to your branch, point the exact link, I'll check.

@lynxlynxlynx
Copy link
Member

Maybe a missing include?

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 17, 2022

Maybe a missing include?

Where exactly...?

@lynxlynxlynx
Copy link
Member

Is there an {% include relurl.html %} somewhere before the call in that file or where it gets included itself?

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 17, 2022

Where? Push to your branch, point the exact link, I'll check.

Done.

The url {{ '/file_formats/ie_formats/eff_v2.htm' | prepend: relurl }} leads to the aforementioned error...

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 17, 2022

Is there an {% include relurl.html %} somewhere before the call in that file or where it gets included itself?

There is one in _includes/op-list.html...

@burner1024
Copy link
Contributor

Yes, it's not working. Apparently collection content gets evaluated before the inclusion.

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 17, 2022

Yes, it's not working. Apparently collection content gets evaluated before the inclusion.

Ok, so what now...?

@burner1024
Copy link
Contributor

I quickly tried liquify/markdownify filters, didn't help, but maybe @lynxlynxlynx has a better idea.

If eventually opcodes are going to be moved to _data, can just leave them as is for now, and fix later.

@lynxlynxlynx
Copy link
Member

Put the include in the opcode file itself, then it should work.

@burner1024
Copy link
Contributor

Right, except there's hundreds of them, not a very clean solution.

@lynxlynxlynx
Copy link
Member

Most don't link anywhere, so it's fine in the interim. Easy to automatically remove when a better solution is available too.

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 18, 2022

Put the include in the opcode file itself, then it should work.

Done.

@burner1024
Do you want me to remove <code> tags when using <a href>...?

@lynxlynxlynx
Copy link
Member

op335 still needs updating

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 18, 2022

op335 still needs updating

Done.

@lynxlynxlynx lynxlynxlynx merged commit 93b3f2c into Gibberlings3:master May 18, 2022
@lynxlynxlynx
Copy link
Member

Thanks and thanks for the patience!

@burner1024
Copy link
Contributor

burner1024 commented May 18, 2022

@4Luke4

  • I don't particularly care.
  • I do think that in most cases it's not necessary, and doesn't really add anything to the page. In some cases, maybe it actually detracts from content (like the timing mode view, which is so littered with markup that I find it hard to read).
  • If you do that, though, better put code inside the href, not the other way around. Because when you convert html to markdown, <code><a href=x>y</a></code> turns into `[x](y)`, which when processed by jekyll stays verbatim. Whereas [`x`](y) turns into a proper link.

@lynxlynxlynx

  1. Should I send a pull adding relurl to all opcodes?
  2. To be clear, is moving opcodes to data in the plan or not? If yes, I'll probably eventually send a pull for that as well, when I find the time to make the conversion. Shouldn't be too hard, considering it's pretty much structured, mostly it'll be html > markdown thing, although that could bring a few surprises.

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 18, 2022

  • Whereas [`x`](y) turns into a proper link.

True, but then x will no longer be highlighted in blue...
As a result, I think I'll remove the <code> tags...

@burner1024
Copy link
Contributor

burner1024 commented May 18, 2022

It should, though.

@4Luke4
Copy link
Contributor Author

4Luke4 commented May 18, 2022

It should, though.

Yeah, me dumb: I had forgotten to update my fork to include your latest changes to _sass/_general.scss...

@lynxlynxlynx
Copy link
Member

@burner1024

  1. I don't think that's needed, we can do it as we start using more and more relurl.
  2. You can go ahead; I see we already discussed this a bit in Machine readable format #84 (concluding the notes should be left inline).

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.

3 participants