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

Add JSON Page #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

optikfluffel
Copy link
Contributor

@optikfluffel optikfluffel commented Jul 26, 2022

TODO

  • The nice icon is still missing, should I try to build one or can you generate those PNGs somehow? :)

Done

  • example.json is copied from www.json.org/example.html.
  • I had to use the git url in mix.exs, since makeup_json doesn't seem to be published on Hex yet.
  • I also changed language.html.eex to use @language.title instead of the name, because "JSON Language" didn't look right to me.

closes #3, when finished

@josevalim
Copy link
Contributor

Hi @lkarthee, is there anything that can help the JSON makeup ship?

@josevalim
Copy link
Contributor

The nice icon is still missing, should I try to build one or can you generate those PNGs somehow? :)

Ah, good call. We will separate the background from the icon, to make the process easier. Please hold on. :)

I also changed language.html.eex to use @language.title instead of the name, because "JSON Language" didn't look right to me.

Can you please submit a separate PR for this so we get it up ASAP? I assume other languages may require it.

@optikfluffel
Copy link
Contributor Author

Can you please submit a separate PR for this so we get it up ASAP? I assume other languages may require it.

The .title was already present in every language before.

@josevalim
Copy link
Contributor

Ah, good call. We will separate the background from the icon, to make the process easier. Please hold on. :)

This is done!

The .title was already present in every language before.

Ack! I meant the change you did will likely be required for EEx/DIFF too (as EEx or DIFF language are also weird).

@optikfluffel
Copy link
Contributor Author

Yes, they'd have to also set the title attribute when adding to MakeupDemo.Languages.languages/0, but since every other language so far has that anyway I thought this wouldn't matter that much. But I can still revert this part and make a separate PR if you prefer.

@josevalim
Copy link
Contributor

I just wanted to merge it sooner but we can also wait until someone does/needs the same change. So no worries.

@optikfluffel
Copy link
Contributor Author

Rebased, removed the change in language.html.eex and added an icon.

@josevalim
Copy link
Contributor

Beautiful, let's see if we can get makeup_json released and ship it. Thank you!

Do you want to PR the language.html.eex change or should I go ahead and change it?

@lkarthee
Copy link

lkarthee commented Sep 8, 2022

Sorry @optikfluffel @josevalim - I missed notification.

@josevalim I have been using makeup_json for sometime - I did not encounter any issues. It has basic tests like I mentioned in this issue - elixir-makeup/makeup_json#4 .

@josevalim
Copy link
Contributor

@lkarthee awesome! Could you please go ahead and release it then? :)

@lkarthee
Copy link

lkarthee commented Sep 8, 2022

Sure will do it this weekend.

@lkarthee
Copy link

Released it on hex. Sorry for the delay.

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.

Add JSON page
3 participants