feat: add pages mockup #2
Conversation
rjborba
commented
Jul 7, 2024
rjborba
commented
ORM-1452 Create pages Mockup
Create mockup for the most important pages so we can check the components interface and see what we have left |
| @@ -5,7 +5,6 @@ defineCustomElements() | |||
|
|
|||
| /** @type { import('@storybook/html').Preview } */ | |||
| const preview = { | |||
| tags: ['autodocs'], | |||
| // TODO: Theme class should be a variable | |||
| decorators: [(story) => `<div id="orama-ui" class="theme-dark">${story()}</div>`], | |||
There was a problem hiding this comment.
btw I added a linear to track the problem with the theme class, you can reference it in the comment https://linear.app/oramasearch/issue/ORM-1455
| @@ -9,11 +9,16 @@ export default meta | |||
| type Story = StoryObj | |||
|
|
|||
| const Template = (args) => { | |||
| return `<orama-chat ${{...args}}></orama-chat>` | |||
| return ` | |||
| <div style="height: 800px; width: 360px; border: 1px solid white;"> | |||
There was a problem hiding this comment.
memo: It would be nice to add the storybook addon to preview mobile version, so we can add multiple screen resolutions to easily test the responsive behaviour (this kind of testing will be recurring for searchbox and other components). I found this but there may be others: https://storybook.js.org/addons/storybook-mobile-addon.
Added this ticket for reminder: https://linear.app/oramasearch/issue/ORM-1456/storybook-addon-to-preview-mobile
| @@ -29,6 +29,7 @@ export class Input { | |||
| @Prop() label?: InputProps['label'] | |||
| @Prop() labelForScreenReaders?: InputProps['labelForScreenReaders'] | |||
|
|
|||
| // TODO: It probably should be a prop as well | |||
There was a problem hiding this comment.
I'm currently completing the Input in my branch, that will fix this. You can remove this comment if you want
| .message-actions { | ||
| display: flex; | ||
| // TODO: Is it ok to be fixed? | ||
| margin-top: 20px; |
There was a problem hiding this comment.
yes I think it's ok in this case! but better to use the pxToRem function
| padding: var(--spacing-m, $spacing-m); | ||
| margin: 0 var(--spacing-l, $spacing-l); | ||
|
|
||
| .message-actions { |
There was a problem hiding this comment.
If we don't strictly need nesting classes, I think it's better to keep this out of .message-wrapper
| justify-content: end; | ||
|
|
||
| > * { | ||
| cursor: pointer; |
There was a problem hiding this comment.
curiosity: why do we need the universal selector here? Putting this rule right under .message-actions did not cover all the cases?
| flex-grow: 0; | ||
| flex-shrink: 0; | ||
|
|
||
| & ::after { |
There was a problem hiding this comment.
maybe a typo here, a space afer &
| .source { | ||
| position: relative; | ||
| overflow: hidden; | ||
| width: 180px; |
There was a problem hiding this comment.
not sure about this width here (better using pxToRem wither way), I will check the preview to better understand the use case
| <div class="sources-wrapper"> | ||
| <div class="source"> | ||
| {/* TODO: We should be able to costumize internal UI components through classes. We probable need an apporach like classe merge of Tailwind */} | ||
| <orama-paragraph class="source-title">Title title title</orama-paragraph> |
There was a problem hiding this comment.
I suppose you added classes here because the style was different by the one defined for our typo elements, but I would try to use them if possible. We can check with Angela in case there are important differences, so we can avoid adding extra classes, if not strictly necessary
| <div class="footer-wrapper"> | ||
| <form onSubmit={this.handleSubmit}> | ||
| <div style={{ display: 'flex' }}> | ||
| <orama-textarea |
There was a problem hiding this comment.
we should define a label for screen readers only here
| </orama-textarea> | ||
| </div> | ||
| </form> | ||
| <div class="spacer" /> |
There was a problem hiding this comment.
If possible, I would avoid adding html tags to handle spacings, I think in this case we can add margin to the form. wdyt?
| import { chatContext } from '../context/chatContext' | ||
| import { searchContext } from '../context/searchContext' | ||
|
|
||
| const LIMIT_RESULTS = 10 |
There was a problem hiding this comment.
memo for next improvements: this value should be customizable as other search paramteres
|
|
||
| // TODO: I'm not sure if this is the best way to do this | ||
| * { | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
this would affect the whole hosting application (I think in this case, it wouldn't be a big deal, but I would avoid it), I did something like
#orama-ui {
// style here
*, *:before, *:after{
box-sizing: border-box;
}
}wdyt?
| return ( | ||
| <Host> | ||
| <div class="message-wrapper"> | ||
| <div>{this.message.content}</div> |
There was a problem hiding this comment.
I see this renders a text, so better to wrap in an inline text element (p is ok) unless there are different cases
