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

Include close icon styles in classless #400

Merged

Conversation

asbjornu
Copy link
Contributor

@asbjornu asbjornu commented Aug 27, 2023

Include the .close icon styling in the classless stylesheet. Fixes #399.

@lucaslarroche
Copy link
Member

@asbjornu I strongly prefer keeping the classless version without any classes.
If you need .close, you can use the regular pico.css file or add the class manually to your custom styles.

@lucaslarroche lucaslarroche added the wontfix This will not be worked on label Dec 28, 2023
@asbjornu
Copy link
Contributor Author

asbjornu commented Dec 28, 2023

@lucaslarroche, I understand that "classless" preferably should not include any classes. Still, as explained in #399, I think feature parity between the "classless" and "classy" versions of Pico is more important than the classless version being guaranteed not to contain a single class. Could we find a compromise so the .close styles are included, somehow?

Perhaps applied to an element or different attribute instead of a class name? How about using the prev link relation?

prev

Refers to the previous document in an ordered series of documents.

In CSS and HTML respectively, it could look like the following:

dialog {
  article {
    a[rel="prev"] {
      /* Current .close styles */
    }
  }
}
<dialog>
  <article>
    <a rel="prev" href="#close" aria-label="Close"></a>
  </article>
</dialog>

An alternative to the prev link relation could be up:

up

…May be used to reference a resource where parent entries of an entry or a feed may be found.

…or start:

start

Refers to the first document in a collection of documents.

Thoughts?

@lucaslarroche
Copy link
Member

lucaslarroche commented Dec 28, 2023

@asbjornu,

a[rel="prev"]

I like it. Great idea 🙂.

This is a breaking change.
But we could ship it on the v2, currently v2.0.0-alpha1.
I was about to release the v2.0.0-rc1 version.

Would you update your PR (or open a new one) to ship this change against the v2 branch?

Here is the file:
https://github.com/picocss/pico/blob/v2/scss/components/_modal.scss

Note: v2 is using yarn. You can use yarn build and yarn dev

@asbjornu asbjornu changed the base branch from master to v2 December 28, 2023 11:55
@asbjornu asbjornu force-pushed the include-close-button-in-classless branch from 8d2f7b5 to b644b78 Compare December 28, 2023 11:55
@asbjornu
Copy link
Contributor Author

@lucaslarroche great! The change is made and the PR is rebased against v2.

@asbjornu asbjornu changed the title Include .close button in classless Include close button styles in classless Dec 28, 2023
@asbjornu asbjornu changed the title Include close button styles in classless Include close icon styles in classless Dec 28, 2023
@lucaslarroche lucaslarroche self-requested a review December 28, 2023 12:41
@lucaslarroche lucaslarroche removed the wontfix This will not be worked on label Dec 28, 2023
@asbjornu asbjornu force-pushed the include-close-button-in-classless branch from b644b78 to 88c1a6b Compare December 28, 2023 13:03
@asbjornu asbjornu changed the base branch from v2 to master December 28, 2023 13:03
@asbjornu
Copy link
Contributor Author

@lucaslarroche, or should I rebase against dev so this can be included in #419? 🤔

@lucaslarroche lucaslarroche changed the base branch from master to dev December 28, 2023 13:13
@lucaslarroche
Copy link
Member

@asbjornu,

I was also thinking of replacing the regular version.

I initially wanted to say that we could also replace .close with a[rel='prev'] on the regular — not classless — version on the v2.
But I'm also okay with shipping it to the v1 version because it's not a breaking change any more.
Sorry for the confusion.

In this case, it will be shipped against dev 😬 and released with 1.5.11 on master.

So, if you want, you can also open a second PR against v2 🙂

@lucaslarroche
Copy link
Member

@lucaslarroche, or should I rebase against dev so this can be included in #419? 🤔

Yes

@lucaslarroche lucaslarroche mentioned this pull request Dec 28, 2023
@asbjornu
Copy link
Contributor Author

I initially wanted to say that we could also replace .close with a[rel='prev'] on the regular — not classless — version on the v2.

I see.

But I'm also okay with shipping it to the v1 version because it's not a breaking change any more.

Ok, cool. Then I think it's probably safest to not introduce the a[rel=prev] selector in the "classy" (default) CSS, no?

Sorry for the confusion.

No problem! 😃

In this case, it will be shipped against dev 😬 and released with 1.5.11 on master.

Nice!

So, if you want, you can also open a second PR against v2 🙂

Sure!

@lucaslarroche, or should I rebase against dev so this can be included in #419? 🤔

Yes

Ok! 👍🏼

@asbjornu asbjornu force-pushed the include-close-button-in-classless branch from 88c1a6b to 4e35c62 Compare December 28, 2023 13:20
@asbjornu asbjornu force-pushed the include-close-button-in-classless branch from 4e35c62 to d85d2f1 Compare December 28, 2023 13:24
@lucaslarroche
Copy link
Member

@asbjornu,

It's probably safest to not introduce the a[rel=prev] selector in the "classy" (default) CSS

Ok, approved, I will merge

Copy link
Member

@lucaslarroche lucaslarroche left a comment

Choose a reason for hiding this comment

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

Thanks!

@lucaslarroche lucaslarroche merged commit 873ad1b into picocss:dev Dec 28, 2023
@asbjornu asbjornu deleted the include-close-button-in-classless branch December 28, 2023 13:31
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.

Missing close icon in classless stylesheet
2 participants