-
Notifications
You must be signed in to change notification settings - Fork 502
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
👌 IMPROVE: Screenshots & Formatting #84
base: master
Are you sure you want to change the base?
Conversation
I appreciate the effort here but this is going to be resolved by #80. Thank you for contributing! |
If you want, you can resubmit just the README changes and we can look at that. |
I really like this design! Also: With the code snippet (really nice idea), we have to write the name of the theme 3 times. Maybe we could make a |
For me personally, I have to see the theme embed and it's colors closest to
how they would look in real life on my site. Bigger screenshots warrant
that.
The font size is huge so that it's more accessible and easy for people with
partially impaired vision to decide on which theme they want to use.
I know about the #80 a modified version of that PR is what I used to
generate this page.
Another point that the bigger screenshots help with is the background
colors.
I ask you to kindly reconsider this PR.
…On Wed, Sep 4, 2019, 10:33 PM Michael Schmidt ***@***.***> wrote:
I really like this design!
The thing I would have to criticize is that the larger screenshots take up
a lot of screen space which makes that page quite long. If we do it like
this, the *available themes* section should be the last looking at #80
<#80>.
Also: With the code snippet (really nice idea), we have to write the name
of the theme 3 times. Maybe we could make a themes.json (similar to
components.json) and generate the available themes section?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#84?email_source=notifications&email_token=AAHKNBO34PJYGLQKJVVW2STQH7WPLA5CNFSM4ITULXAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD54LNKQ#issuecomment-528004778>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHKNBMP3UJRL4Q6O2XOYFTQH7WPLANCNFSM4ITULXAA>
.
|
I'm going to hold this for a minute and get #80 in. Once that's landed, could you merge/rebase this PR so the changes you made to the build process are included? Then we can adjust the sizes if we like. |
@ahmadawais Since the available themes section is the last one now, I think the larger screenshots are ok. |
@ahmadawais #80 is in. You can merge that up to this branch and push your modifications here so we can get the new README design + new screenshot process in at once. Thanks! |
Let's consider this in a separate PR, although given how infrequently this README changes, I'm not really sure it's necessary. |
Just an idea. But that's definitely its own PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahmadawais
From my perspective, this PR just needs rebasing and a few minor changes.
You also have to incorporate your changes to the screenshot task, so that we can recreate the screenshots anytime.
If you need any help, I'll be happy to help.
|
||
To use one of the themes, just include the theme's CSS file in your page. Example: | ||
To use one of the themes, include theme's `css` file (present in the [themes directory](themes)) in your page. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To use one of the themes, include theme's `css` file (present in the [themes directory](themes)) in your page. For example: | |
To use one of the themes, include the theme's `css` file (present in the [themes directory](themes)) in your page. For example: |
</body> | ||
<head> | ||
... | ||
<!-- <link href="prism-[theme-name-here].css" rel="stylesheet" /> --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this comment? Isn't the line below enough?
@@ -1,80 +1,214 @@ | |||
# Prism themes | |||
# Prism.js Themes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just call ourselves "Prism", so I think we should go with the old heading.
This PR includes an improved formatting of the
README.md
file with better/bigger screenshots.❯❯ Live Demo →
Looking forward, peace! ✌️