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 the dir HTML attribute #72

Merged
merged 5 commits into from
Mar 31, 2023
Merged

Conversation

alobaili
Copy link
Contributor

@alobaili alobaili commented Jun 20, 2021

Since I will be using this package to build my personal website, which will be supporting English and Arabic, I figured the best way to thank you was to add this small contribution.

I added the ability to add the dir attribute in both the Node and Component based APIs.

It is a global attribute that can be added to any HTML elemnt (technically any element including non-HTML, but I didn't focuse on XML in this PR).

I added a new enum called Directionality (the name is inspired from the spec linked above) to hold the three possible values. I made the case names clearer to read and added the actual HTML values as a raw value to each case.

public enum Directionality: String {
    case leftToRight = "ltr"
    case rightToLeft = "rtl"
    case auto = "auto"
}

users can use this like so:

// Support Arabic for the entire document.
let html = HTML(
    .lang(.arabic),
    .dir(.rightToLeft),
    .body(.component(Home())
)

// Or for a specific element.
struct Home: Component {
    var body: Component {
        H1("أهلًا بالعالم!").dir(.auto)
    }
}

I hope I added them to the correct extensions.

Your discussions and comments are always welcome.

@alobaili
Copy link
Contributor Author

Hello @JohnSundell, could you please review my PR?

Thanks for your time and support.

@alobaili
Copy link
Contributor Author

Hello @JohnSundell, I'm sorry for any inconvenience. Is there a timeframe we can expect where you usually review PRs for Plot?

Thank you.

@alobaili
Copy link
Contributor Author

alobaili commented Sep 24, 2021

@MrSkwiggs regarding naming the method dir(_:) instead of directionality(_:). This is to match existing names in Plot like H1 and lang. I noticed external names tend to match their HTML counterparts.

What do you think?

@MrSkwiggs
Copy link
Contributor

Makes sense, just got confused initially thinking this was about directories 😆

Repository owner deleted a comment from ggoraa Mar 26, 2022
@majortom64
Copy link

@JohnSundell is there a problem with this PR preventing its merge? I have a need for this as well, but do not want to use it if there is an issue.

@majortom64
Copy link

@JohnSundell Checking on this again. It has been about 18 months since this was submitted. I really need this, and I am wondering if there is an issue preventing it from being accepted.

Component modifiers should use Swift's default naming conventions, and not include abbreviations that are commonly used within HTML's naming conventions.

Co-authored-by: Dorian <[email protected]>
Copy link
Owner

@JohnSundell JohnSundell left a comment

Choose a reason for hiding this comment

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

A really great addition to Plot, thanks so much @alobaili, and I'm really sorry for taking so ridiculously long to review this!

@JohnSundell
Copy link
Owner

I accepted @MrSkwiggs suggestion to rename dir to directionality for the Component-based modifier API, since components generally use Swift's default naming conventions, while nodes match their underlying HTML names. Seems like the tests are failing, I'll look into that this evening and will get this PR merged. Again, my sincere apologies for taking so long to review this!

@JohnSundell
Copy link
Owner

@alobaili Alright, I just needed to update the unit tests for the new directionality API naming as suggested by @MrSkwiggs. I also pushed your fork's master branch to the lastest master of this repo, I hope that's OK. Let me know otherwise and I'll revert back to the commit it was on before. But with that in place this is now ready to be merged in 😀

@alobaili
Copy link
Contributor Author

@JohnSundell Amazing! I'm glad everything worked out, and thank for accepting my PR. This is one of the very first contributions I made to open source projects that contained actual code 😄.

Thank you for updating my fork as well 🙏.

@JohnSundell JohnSundell merged commit 42ab862 into JohnSundell:master Mar 31, 2023
@JohnSundell
Copy link
Owner

@alobaili Glad to hear it 😀 Really appreciate your pertinence in waiting for me to FINALLY have some time to go through these Plot PRs and review them. Thanks again for implementing this feature, I'll include it in a new release shortly 👍

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.

4 participants