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

feat: add preview element #1676

Merged
merged 5 commits into from
Jun 3, 2024
Merged

feat: add preview element #1676

merged 5 commits into from
Jun 3, 2024

Conversation

akomiqaia
Copy link
Contributor

@akomiqaia akomiqaia commented May 30, 2024

Describe your changes

Add preview component for astro web page

Issue ticket number and link

OR-717

Checklist before requesting a review

  • I have updated relevant docs.

@akomiqaia akomiqaia requested a review from a team as a code owner May 30, 2024 16:04
Copy link
Contributor

@notlee notlee left a comment

Choose a reason for hiding this comment

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

Love it, love it

apps/for-everyone-website/README.md Show resolved Hide resolved
apps/for-everyone-website/README.md Outdated Show resolved Hide resolved
apps/for-everyone-website/astro.config.mjs Show resolved Hide resolved
apps/for-everyone-website/package.json Outdated Show resolved Hide resolved
encoding: 'utf8',
});

const regex = /\/\/\s*<preview>([\s\S]*?)\/\/\s*<\/preview>/;
Copy link
Contributor

Choose a reason for hiding this comment

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

Between preview comments this matches on white-space and non-white-space – effectively anything (visualisation)

Could we make it a little simpler with?

/\/\/\s*<preview>(.*?)\/\/\s*<\/preview>/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's a very tiny detail that people might miss and let's why I included it so if someone doesn't use space between comments than it will still work

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're talking about different things. My point was we could write ([\s\S]*?) as (.*?) and I believe get the same result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh i see :D sorry i thought you meant different thing

Copy link
Contributor

@notlee notlee May 31, 2024

Choose a reason for hiding this comment

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

Actualllllllyyy.... it looks like . doesn't capture line breaks where \s\S I think will, that's something to watch out for, maybe ignore this.

@notlee notlee self-requested a review May 31, 2024 14:25
Copy link
Contributor

@notlee notlee left a comment

Choose a reason for hiding this comment

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

🚀 🚀 🚀 🚀 🚀 🚀

@notlee notlee temporarily deployed to origami-webs-or-717-add-twtxnd June 3, 2024 11:45 Inactive
@akomiqaia akomiqaia temporarily deployed to origami-webs-or-717-add-twtxnd June 3, 2024 11:47 Inactive
@notlee notlee merged commit 292d86e into main Jun 3, 2024
9 checks passed
@notlee notlee deleted the OR-717-add-preview-element branch June 3, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants