Create page link UI

[NobodysNightmare]

Ticket

https://community.openproject.org/wp/73353

[github-actions]

Caution

The provided work package version does not match the core version

Details:

• Work package URL: https://community.openproject.org/wp/73353
• Work package version: not set
• Core version: 17.6.0

Please make sure that:

• The work package version OR your pull request target branch is correct

[Kharonus]

🔴 I do not agree with the term Link for the submit action of this dialog. The figma uses Add and I find it a better alternative. Maybe even written out: Add wiki page link. In the end, this is a UX question, but the indicator, that you had to create a new locale key for it, is also there.

[NobodysNightmare]

I am happy to use whatever someone else decides for me. However, this is frustrating.

I used 100% of the initial Figma, then showed stuff to @dominic-braeunlein and he pointed out that the action menu items should be called "Link ..." (see the very first comment on this PR). I also want to point out that Dominic changed the expected texts in the Figma in one place.

Then looking at the remaining Figma designs, I noticed that the text was not updated everywhere, but that the Dialog had the name of the action menu for a title, so I changed the dialog title to "Link ...".

Now finally I had a dialog titled "Link ..." but the primary action was "Add". This didn't make sense to me, so I changed the button as well.

tl; dr: We should consistently change the mockups to whatever and then do that.

[Kharonus]

ololol... I see your frustration. :D wasn't aware, that dominic already overwrote figma.

[Kharonus]

🟡 I do not really know, why you people prefer the "every line a <%=" approach, over one big enclosing <%= ... =>.

I know, there are cases where it actually IS necessary, like in the usage of the primer stack with multiple renders at the same level. Or the dialog footer in this very file. But in all the other cases it adds way to much noise for my taste. WDYT?

[NobodysNightmare]

I usually go for one extreme or the other. Everything in a single <%= %> is fine. However, if that's not possible, I usually "give up" and separate all lines.

As you pointed out the footer requires this. I could've probably single-blockified the dialog.with_body. However, you put this comment onto the render for the entire dialog. I am not sure whether this is syntactically possible (since it would cover a block that spans over the footer, where we need multiple renders). Even if it was possible, I'd need to see the result to judge how I like it.

Feel free to transform this dialog into your preferred form and show it to me once there are no more syntax errors.

[Kharonus]

So, the two options are:

<%= render(Primer::Alpha::Dialog.new(id:, title: t(".title"), size: :large, **system_arguments)) do |dialog| %>
  <% dialog.with_body do %>
    <%= primer_form_with(**form_options) do |form| %>
      <%= render(Wikis::LinkExistingWikiPageForm.new(form)) %>
    <% end %>
  <% end %>
  <% dialog.with_footer do %>
    <%= render(Primer::Beta::Button.new(data: { "close-dialog-id": id })) { t("button_cancel") } %>
    <%= render(Primer::Beta::Button.new(scheme: :primary, form: form_id, type: :submit)) { t("button_link") } %>
  <% end %>
<% end %>

versus

<%=
  render(Primer::Alpha::Dialog.new(id:, title: t(".title"), size: :large, **system_arguments)) do |dialog|
    dialog.with_body do
      primer_form_with(**form_options) do |form|
        render(Wikis::LinkExistingWikiPageForm.new(form))
      end
    end
    dialog.with_footer do
      concat(render(Primer::Beta::Button.new(data: { "close-dialog-id": id })) { t("button_cancel") })
      concat(render(Primer::Beta::Button.new(scheme: :primary, form: form_id, type: :submit)) { t("button_link") })
    end
  end
%>

As you notice, you CAN make it work in the footer, too, but you need a helper method. Now, the rest is personal taste. ^^ And I won't force you to use any of those. I just stated, that at least for me, my eyes stumble over those dozens of <%= ... %>.

[NobodysNightmare]

yeah, fair. Though I always feel like concat is a hack, so I avoided using that too.

Your first version is the same as my current code, isn't it?

I mean, we could optimize that at least a bit, if we wanted to:

<%= render(Primer::Alpha::Dialog.new(id:, title: t(".title"), size: :large, **system_arguments)) do |dialog| %>
  <% dialog.with_body do
    primer_form_with(**form_options) do |form|
      render(Wikis::LinkExistingWikiPageForm.new(form))
    end
  end %>
  <% dialog.with_footer do %>
    <%= render(Primer::Beta::Button.new(data: { "close-dialog-id": id })) { t("button_cancel") } %>
    <%= render(Primer::Beta::Button.new(scheme: :primary, form: form_id, type: :submit)) { t("button_link") } %>
  <% end %>
<% end %>

[Kharonus]

Nah, I wouldn't enforce a partial change, only if you want to.

IMHO concat is easier on the eyes, then the rest, but like said before, this is highly subjective. From the hack perspective: concat is essentially the same as an <%= => tag. It appends content to the output buffer. It is design to be used inside a non-output tag.

[Kharonus]

🟡 Ids directly inserted in the HTML usually have kebab-case in our application.

[NobodysNightmare]

ok

[Kharonus]

🔴 here is the same: the figma currently talks about "adding" wiki pages to the work package, and user facing I'd agree. tomorrow titan dev discussion?

[NobodysNightmare]

Figma agrees with this:

However, as I explained above, it only does so inconsistently (and after edits from Dominic). I'd say UX daily.

[NobodysNightmare]

New words:

• "+ Wiki page:" -> aria label should be "Add wiki page"
• Then "Existing wiki page" (aria label: "Add existing wiki page")
• then "New wiki page" (aria label: "Add new wiki page")
• dialog title: "Add (existing|new) wiki page"
• dialog submit button: "Add"

[NobodysNightmare]

@bsatarnejad Can I quickly solicit your input on this? We agreed one something in a meeting earlier today for which I don't know if it's feasible and/or even supported in Primer.

Given the menu in the screenshot above, we want to set the aria-label for the button to "Add wiki page". This makes sense, because the icon would otherwise not be properly represented.

However, then we decided that the action menu should have a different aria-label than its normal label/text, mostly to re-assert the contextual "add", which is implicit to sighted users but may be less obvious to non-sighted users:

1. Is this actually a best practice? (e.g. label "existing wiki page", but aria-label "add existing wiki page" in the action menu)
2. Would it be the right course of action to add this to the menu item like this:

menu.with_item(
              label: "existing wiki page",
              aria: { label: "add existing wiki page" },
              ...
            )