[59915] Update PageHeaders & SubHeaders in the (rails) project pages (Part 2) #18600
Conversation
bsatarnejad
force-pushed
the
59915-update-pageheaders-subheaders-in-the-rails-project-pages-part-2
branch
from
2b42ff6 to
e1679b6
Compare
There was a problem hiding this comment.
Hi @bsatarnejad
I did not manage to go through all of it yet, but would like to give you some initial feedback already:
- Please extract the more complex PageHeaders into separate components. Especially the one for the wiki/show page is so complex, it has to be extracted. I think a general rule of thumb would be that as soon as you have logic inside, extract it into a component (e.g when having
ifclauses or permission checks) - Please do not use classes for testing but provide test_selectors to the elements which you then access in the class.
- I am wondering whether in this example, the toolbar should read "Seeded project / Wiki / Coole version" . What do you think?
Hey @HDinger thanks for the hints.
|
app/views/wiki/history.html.erb
Outdated
| <% breadcrumb_paths(*(@page.ancestors.reverse.collect { |parent| link_to h(parent.breadcrumb_title), { id: parent, project_id: parent.project } } + [h(@page.breadcrumb_title)])) %> | ||
| <%= | ||
| render Primer::OpenProject::PageHeader.new do |header| | ||
| header.with_title { h(@page.title) } |
There was a problem hiding this comment.
This feels weird.. I'd expect something like "History of {title}" and get rid of that second headline. Or just "History" and represent the title in the breadcrumb
bsatarnejad
force-pushed
the
59915-update-pageheaders-subheaders-in-the-rails-project-pages-part-2
branch
from
aee5379 to
d74f47f
Compare
| <% end %> | ||
| <%= | ||
| render(Primer::OpenProject::SubHeader.new) do |subheader| | ||
| if show_create? |
There was a problem hiding this comment.
This should probably be before calling the component to avoid empty space:
| breadcrumb_for_page(@project, @page) | ||
| ) | ||
| end | ||
| %> |
There was a problem hiding this comment.
The breadcrumb and the title does not match
There was a problem hiding this comment.
I take it from your comment here: #18600 (comment),
'History' for title and page.title in breadcrumb, did I misunderstand?
There was a problem hiding this comment.
Yeah, this is not what I meant.. Normally, the breadcrumb and the title should be the same. The only excpetion is when using the selected_item_font option. So you have two options:
- Both show "History"
- The title shows "History", the breadcrumb shows "Blup: History"
| within('[data-test-selector="wiki-page-header-breadcrumbs"]') do | ||
| expect(page).to have_text(parent_page.title.to_s) | ||
| end | ||
| expect(page).to have_css(".breadcrumb-item", |
There was a problem hiding this comment.
What is the point of testing this? You are already checking the for the title to be part of the breadcrumb above.
There was a problem hiding this comment.
Sorry, It was duplicated, fixed.
Ticket
https://community.openproject.org/wp/59915
What are you trying to accomplish?
Update page header in remaining rails pages