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 embedded-text example #13

Merged
merged 1 commit into from
May 31, 2021
Merged

Conversation

bugadani
Copy link
Member

@bugadani bugadani commented May 30, 2021

This is just the simplest possible example I could think of, that would otherwise be annoying to do in embedded-graphics. Surprisingly, things seem to work :)

Progress towards #8

eg-next/examples/textbox-alignment.rs Outdated Show resolved Hide resolved
eg-next/examples/textbox-alignment.rs Outdated Show resolved Hide resolved
@@ -76,3 +76,8 @@

[![A screenshot of the embedded-graphics version next example called text-transparent](../doc/assets/next/text-transparent.png)](examples/text-transparent.rs)


## [textbox-alignment](examples/textbox-alignment.rs)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the name of the demo. I would like to use the text- prefix, because it is a text related feature and contain embedded-text, because it is a demo for that external create. But text-embedded-text doesn't sound great either.

Copy link
Member

Choose a reason for hiding this comment

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

text-basic-layout is the best I can come up with

Copy link
Member Author

Choose a reason for hiding this comment

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

I was considering merging the example into text-multiline or text-alignment and I didn't like either option. Truth is, this example is a bit of both.

I'm afraid that users would miss the fact the example uses an "external" crate if we keep the names so similar.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe text-textbox?

In the future we could update the README.md to include sections and maybe some description per example. But for now we should just pick any of the suggested names. This can easily be improved later without breaking anything.

Copy link
Member Author

@bugadani bugadani May 30, 2021

Choose a reason for hiding this comment

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

text-textbox seems good enough. Since there exists a single example right now, I've used this name. Later this might turn into a prefix.

We might also use something like text-advanced-... in the future, maybe.

@@ -76,3 +76,7 @@

[![A screenshot of the embedded-graphics version next example called text-transparent](../doc/assets/next/text-transparent.png)](examples/text-transparent.rs)

## [text-textbox](examples/text-textbox.rs)

[![A screenshot of the embedded-graphics/embedded-text version next example called text-textbox](../doc/assets/next/text-textbox.png)](examples/text-textbox.rs)
Copy link
Member

Choose a reason for hiding this comment

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

I guess you didn't use just generate-readmes to generate this file? I don't think it's checked by CI at the moment, but could lead to problems in the future.

Copy link
Member Author

@bugadani bugadani May 30, 2021

Choose a reason for hiding this comment

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

Nah, I'm just using windows ^^. Is there something wrong here? Ah I see, there's a template.

Copy link
Member

Choose a reason for hiding this comment

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

If you have allowed edits by maintainers I can run the script if you like.

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I'd prefer not generating the alt texts. Allow edits is on by default.

Copy link
Member

Choose a reason for hiding this comment

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

OK. Done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you

@jamwaffles
Copy link
Member

I added the check-readmes command but didn't add it into the CI config 🤦.

I also just noticed CircleCI didn't have the "Build forked pull requests" option turned on, which I've now enabled.

@rfuest
Copy link
Member

rfuest commented May 30, 2021

I also just noticed CircleCI didn't have the "Build forked pull requests" option turned on, which I've now enabled.

Great, I was wondering why CI didn't work.

@bugadani
Copy link
Member Author

Aww yessssss, e-t is MSRV 1.43.

@rfuest
Copy link
Member

rfuest commented May 30, 2021

A forgot that e-t has a newer MSRV than e-g. Should we just disable the 1.40.0 build? I honestly don't care about 1.40.0 compatibility anymore and would like to change the MSRV policy as soon as 0.7 is released.

@jamwaffles
Copy link
Member

I'd be happy to bump the MSRV too as I'm also not that bothered by it. Would we be ok just bumping to 1.43 for now and opening an issue in e-g to discuss what the new version/policy should be?

@bugadani
Copy link
Member Author

I think examples are fine on stable only. MSRV is that - minimum supported, the crates can stay as low as they can/want IMO.

@rfuest
Copy link
Member

rfuest commented May 30, 2021

We will also need to consider eg-bdf and eg-seven-segments MSRVs. I think they are 1.44.0, but I haven't checked in a while and without the proc macro they might work on older versions now.

I agree with @bugadani in that we should just require the latest stable version for the examples to keep this simple.

@jamwaffles
Copy link
Member

Sounds good to me! Shall I attach that change to #14 ?

@rfuest
Copy link
Member

rfuest commented May 30, 2021

Sounds good to me! Shall I attach that change to #14 ?

Yes, why not. Do you want to merge this PR in a semi broken state for now?

@bugadani
Copy link
Member Author

Do you want to merge this PR in a semi broken state for now?

There's no rush on this one

@jamwaffles
Copy link
Member

I just merged #14 so this PR should pass its build once rebased.

Co-authored-by: Ralf Fuest <mail@rfuest.de>
@jamwaffles jamwaffles merged commit e0d7801 into embedded-graphics:main May 31, 2021
@jamwaffles
Copy link
Member

Sweet!

@bugadani bugadani deleted the et branch May 31, 2021 14:35
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

3 participants