-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Agent Builder] Landing Page - Change layout and add quick links #235240
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
[Agent Builder] Landing Page - Change layout and add quick links #235240
Conversation
|
Hey @chrisbmar I wanted to ask you if these layout changes will break the auto scrolling? Just because the way I'm doing the |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
jedrazb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested locally works well! some nits
...ugins/shared/onechat/public/application/components/conversations/conversation_grid.styles.ts
Show resolved
Hide resolved
| const gridStyles = css` | ||
| display: grid; | ||
| grid-template-columns: ${sideColumnWidth} minmax(auto, ${contentMaxWidth}) ${sideColumnWidth}; | ||
| grid-template-columns: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is tailwind css when we need it ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've actually never used Tailwind in any larger project. Pretty used to using CSS-in-JS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does tailwind have good utility classes for CSS grid? I can't imagine that's much easier to read/write is it?
| 'xpack.onechat.newConversationPrompt.agentBuilderDocsAriaLabel', | ||
| { | ||
| defaultMessage: 'Read Agent Builder documentation', | ||
| <EuiFlexGroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm can't we use https://eui.elastic.co/docs/components/display/empty-prompt/ here?
I believe this is default style for empty state pages in EUI cc @julianrosado
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think empty prompt will accommodate all this
| {cards.map(({ key, title, description, iconType, path }) => { | ||
| return ( | ||
| <EuiFlexItem key={key}> | ||
| <EuiCard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for a11y
role=button
aria-label=...
this is just div so readers might nor recognize this as clickable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also data-test-subj for full story analytics and synthetics would be good to have
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So an EuiCard with an href does use an <a> element which has role="link", I think that will be picked up properly by screen readers even though it's not the parent element.
It seems like the EUI team has had some discussion around this elastic/eui#2521, citing this article https://inclusive-components.design/cards/
Based on these things, I'm inclined to leave it how it is? I can add the data-test-subj though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay made some adjustments here:
- I'm using
ulandlielements for thelistandlistitemroles to help group the markup a bit better - Added the
data-test-subjattributes to the list and to each item - Added an
aria-labelfor the list
chrisbmar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left you a few comments to consider.
Why did you go with grid over flexbox here? I feel the grid syntax adds a lot of unnecessary complexity - curious about your thinking on the tradeoffs.
Nice work, I think it looks really great and is a much better landing page than before.
...ugins/shared/onechat/public/application/components/conversations/conversation_grid.styles.ts
Show resolved
Hide resolved
...orm/plugins/shared/onechat/public/application/components/conversations/conversation_grid.tsx
Show resolved
Hide resolved
...ugins/shared/onechat/public/application/components/conversations/new_conversation_prompt.tsx
Outdated
Show resolved
Hide resolved
@chrisbmar In general, I like to use flex when the items' layout is being controlled in one dimension, and CSS grid when the items' layout needs to be controlled in both dimensions. Certainly you could achieve the layouts I built with flexbox rather than a grid, but I thought this layout was a good use case for grid. The CSS grid API is definitely a little bit complex, but I also think it's really powerful if you get used to using it. Also! I didn't think about it, but maybe I should have used subgrids for this. It's a new feature and I'm used to having to support some older browsers, but maybe that's not the case for Kibana? |
|
@chrisbmar For the grid template we could also do this, which is pretty cool Then the containers would be styled like
Could do this if you think it makes the column areas clearer |
I think the grid syntax in general is just complex and difficult to read 😄 that's a personal thing though, not a reflection on the code you've written. The new suggestion you shared does seem slightly easier to read, but it's up to you if you want to implement that or you want to try subgrid's - I don't want to hold this PR up on styling, it achieves the same thing at the end of the day and what you've implemented is a great improvement and I'd love to see it in before the cutoff tomorrow. |
...platform/plugins/shared/onechat/public/application/components/conversations/conversation.tsx
Show resolved
Hide resolved
chrisbmar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! Approving now but I think Lee has some requests - but code LGTM. Let me know if you need another approval if you push another commit. Looking forward to seeing this landing :)
💛 Build succeeded, but was flaky
Failed CI StepsTest FailuresMetrics [docs]Module Count
Async chunks
Page load bundle
History
|
| * 2.0. | ||
| */ | ||
|
|
||
| const docsRoot = 'https://www.elastic.co/docs/solutions/search/elastic-agent-builder'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc paths are provided via another way, rather than you needing to hardcode the url for them.
see playground as an example:
- need to create a place for the onechat plugin to set doc_links (x-pack/solutions/search/plugins/search_playground/common/doc_links.ts)
- then the docLinks are set at plugin start (x-pack/solutions/search/plugins/search_playground/public/plugin.ts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chris opened up a pr to put in the support for docLinks :) #235804
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Should I just remove these doc links and wait for the PR that adds all of the links together?
joemcelroy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. one issue with the docs handling however.
…stic#235240) ## Summary <img width="974" height="772" alt="image" src="https://github.com/user-attachments/assets/428b0328-1bf4-48d8-98c6-0ed38ef6adb6" /> ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. --------- Co-authored-by: Elastic Machine <[email protected]>
…5240) ## Summary <img width="974" height="772" alt="image" src="https://github.com/user-attachments/assets/428b0328-1bf4-48d8-98c6-0ed38ef6adb6" /> ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. --------- Co-authored-by: Elastic Machine <[email protected]>
…stic#235240) ## Summary <img width="974" height="772" alt="image" src="https://github.com/user-attachments/assets/428b0328-1bf4-48d8-98c6-0ed38ef6adb6" /> ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. --------- Co-authored-by: Elastic Machine <[email protected]>
Summary
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.