cubing / icons

🖼 Icons for WCA events, unofficial events, and competition-related concepts.
https://icons.cubing.net
MIT License
42 stars 22 forks source link

Document naming conventions and repo structure #108

Open jfly opened 1 year ago

jfly commented 1 year ago

See discussion on https://github.com/cubing/icons/pull/106#issuecomment-1712192607.

We're starting to form some opinions about naming conventions (kebab case, longer, more readable ids). @dmint789 also pointed out that it's really unclear what the "ids" of our icons actually are. For example, we have:

dmint789 commented 1 year ago

Also, we should try to be consistent when it comes to naming events that are extensions of existing events. E.g. 3x3 Blindfolded 2 Person Relay should be 333bf-2-person-relay, since it is simply an extension of the event 3x3 Blindfolded, which has the id 333bf.

lgarron commented 1 year ago

Bringing over the discussion from other issues:

I think we should go with underscores for event names. The only legacy compat concern would be mirror blocks, which is unlikely to be used by a significant number of pages yet.

dmint789 commented 1 year ago

Why do you want underscores over dashes? The way I see it, it makes no difference

lgarron commented 1 year ago

Why do you want underscores over dashes? The way I see it, it makes no difference

Underscores tend to be better for identifiers, since:

For our CSS class names, it would also help isolate the event ID from the icon type prefix.

dmint789 commented 1 year ago

Oh, I see. Well, I don't have a strong opinion on this as long as we keep things consistent and don't mix naming conventions.

dmint789 commented 1 year ago

@jfly Thoughts?

jfly commented 1 year ago

I'm fine with underscores. Let's leave this issue open to track a little more work:

lgarron commented 1 year ago
  • let's actually enforce a regex for event ids

Sounds like a good idea!

  • let's add a blurb somewhere explaining the events vs unofficial folders and how stable we promise the ids to be

I propose:

It might be worth keeping track of some of this in code — https://github.com/cubing/icons/pull/112 can generate TypeScript code to refer to icons, and I also want to include a bit of event metadata in the repo (so you can semantically e.g. go from a speed event to its associated BLD event or team event or vice-versa, etc.).

dmint789 commented 1 year ago

I feel like we should keep things permanent in general, not just for wca events. This should be the last time we make an icon id change in my opinion, because it's not a good strategy to change something as important as the id of an event, even if it's unofficial.

And what's this point about removals? Why would we ever remove an icon?

lgarron commented 1 year ago

I feel like we should keep things permanent in general, not just for wca events. This should be the last time we make an icon id change in my opinion, because it's not a good strategy to change something as important as the id of an event, even if it's unofficial.

I think this is reasonable in principle, but we're almost certainly going to run into edge cases over time. I'm not arguing that we should ever do it lightly, just that we set a baseline for the requirements in case it ever happens.

And what's this point about removals? Why would we ever remove an icon?

The main one I have in mind is removing something from unofficial if it ever becomes official. I still don't know if that's a good idea, but again just setting a baseline.

dmint789 commented 1 year ago

Well, then that's not removing an icon, that's just changing which folder it's located in, which I think is totally fine.

And yeah, I agree, it should be possible to rename an icon, but that should be a super rare thing and have very good reasons for doing so. In my case, I use the same id for my events in the db as the icon ids, because I like things to be standardized. But now that we're gonna be renaming all icons with dashes, I'm gonna have to run scripts to update my events, as well as all of the results and rounds for the changed events. So it's not insignificant. And for the same reason I feel like mirror blocks might need to still have a copy using the old kebab-case naming scheme after we make this change.

dmint789 commented 1 year ago

Actually, now that I think about it, the people who used mirror blocks aren't gonna be affected until they update to the new npm package version, right? But my point still stands, changes to icon names must be very rare.

jfly commented 1 year ago

I agree that we should strive to not change icon names, but in terms of what we commit to, I like @lgarron's proposals.

I worry we're not all on the same page about icon names vs event ids. They don't feel like the exact same thing to me. For example:

In other words: this repo defines icon names, and the names lead humans to make a guess about what WCA event ids those might correspond to if the WCA were to ever adopt the event as an official event.

dmint789 commented 1 year ago

@jfly wait, is this repo not upstream from the wca? I assumed the wca frontend used this repo for icons. And I thought it would be good for us to set the standards for new event ids.

jfly commented 1 year ago

You are correct that the WCA website uses this project. I think we have a good relationship with the WST and WRT, so I think if any of the unofficial events in this repo ever do become official, there's a good chance they'd be willing to reuse the ids we have here. I just am a little anxious about using strong language like we're "setting the standard for new event ids". That decision is ultimately up to the WST and WRT.

dmint789 commented 1 year ago

@jfly yeah, of course I agree that if it did come to that and they used a different id, we'd rename it, but that would also be at the same time as changing the folder from unofficial to event and it would be super rare, so it's okay, I think.