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: added link component #266

Merged
merged 8 commits into from
Mar 3, 2024
Merged

feat: added link component #266

merged 8 commits into from
Mar 3, 2024

Conversation

raaymax
Copy link
Collaborator

@raaymax raaymax commented Feb 26, 2024

This needs double check, I added dynamic list of options for components.

const options = computed(() => {
const field = templateField.value;
if (field.options) {
return typeof field.options === "function"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @FabienArcellier check this out

@raaymax worked on a dynamic list of options. You can pass it hardcoded options (as we do now), but you can also pass it a function to generate the options.

This was one of the challenges I saw for the Reuse Component component. So with this we're halfway there. I've asked him to work on Reuse Component.

@raaymax for your reference #215

Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way I'm talking specifically about selecting the id of the component that we want to reuse.

ui/src/core_components/content/CoreLink.vue Outdated Show resolved Hide resolved
ui/src/core_components/content/CoreLink.vue Outdated Show resolved Hide resolved
ui/src/core_components/content/CoreLink.vue Outdated Show resolved Hide resolved
ui/src/core_components/content/CoreLink.vue Outdated Show resolved Hide resolved
url: {
name: "URL",
type: FieldType.Text,
desc: "A valid URL.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was quite confused by the list of pages at first, when I got to try this component. Took me a while to be like "Ah, of course, the pages are accessible via #".

I love the new dynamic options capability, but it makes you think the right options are the ones listed, when I expect most people to enter a "standard" URL e.g. "github.com".

If we keep it:

  • I'd make it clearer in the desc like, "Specify a URL or choose a page."
  • I'd set the default to https://streamsync.cloud. Bit of self promotion but it'd make it clear right away that yes, you can include "normal" URLs and not just #fragments.

I'm also not sure how it'd behave when the list is empty, needs checking. I expect most apps to not have any keyed pages.


Also, something to keep in mind... The hash can contain parameters that will be lost when switching pages this way.

https://www.streamsync.cloud/page-routes.html#routes-with-parameters

For example if you have /#detailPage/product_id=32&country=AR and you switch via event handler or backend action it'll preserve the rest of the hash.

If someone types #mypage then they literally asked for the parameters to be removed, but if they select it from a list they might have the expectation that it behaves like other page-jumping functionality.


I'm more inclined towards not keeping the dynamic options here, but they'd certainly come in handy for Reuse Component.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The logic that manages the hash is in CoreRoot.vue if you're interested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that we've been explicitly asked to link pages. So maybe let's try how it'd look like with the minor changes I suggested under "If we keep it".

desc should probably be "Specify a URL or choose a page. Keep in mind that you can only link to pages for which a key has been specified."

Keen to hear your thoughts. Apologies for the verbosity, you probably weren't expecting an essay after submitting a PR for an a href.

Copy link
Collaborator Author

@raaymax raaymax Feb 28, 2024

Choose a reason for hiding this comment

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

I think it's just how these kinds of links work. Personally, it would be really confusing for me if the link appended parameters at the end of the URL. I expect that most people using this app will have at least a really basic knowledge about HTML, and <a href> is probably the first thing they learn.

const options = computed(() => {
const field = templateField.value;
if (field.options) {
return typeof field.options === "function"
Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way I'm talking specifically about selecting the id of the component that we want to reuse.

@@ -0,0 +1,86 @@
<template>
<div class="CoreLink">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you're working on links, can you please check how the Markdown links are working, if everything is alright with them?

I assume that with a Text component, markdown mode, I can [hello](https://streamsync.cloud).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just checked and yes - you can. You can also [Next page](#next-page) to navigate between pages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So it's basically the same, only difference is that you can select page in component and define rel and target.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for checking!

Yes they're quite similar, that's what kept me from creating this component. However, now that I think more about it:

  • Agreed, there's the rel and target
  • You can change the color
  • It's more semantic. If you see a component tree with 3 Link, you it's self-explanatory. You see 3 Text, could be anything.
  • It's more easily discoverable. "I need to create a link, so I'll create a Text component which probably supports markdown and I'll []() my way into it." -Probably no one ever, at least not on their first try.

By the way there's the possibility of opening URLs from the backend via backend actions too.

return Object.fromEntries(
ss
.getComponents("root", true)
.map((page) => page.content.key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Careful with this one. Like I was saying earlier not all pages will have keys. In fact I expect most pages to not have keys.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah sorry didn't see the next line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thats why there is filter in the next line

@ramedina86 ramedina86 merged commit 26eb89a into writer:dev Mar 3, 2024
13 checks passed
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.

2 participants