Add sticky date markers #2731
Add sticky date markers #2731
Conversation
holmesworcester
changed the title
holmesworcester
changed the title
holmesworcester
force-pushed
the
feature/sticky-date-markers
branch
from
7816bc1 to
1a399db
Compare
|
|
| cy.get(floatingDateSelector) | ||
| .should('be.visible') | ||
| .invoke('text') | ||
| .should('contain', '28 Oct') |
There was a problem hiding this comment.
Is there a way we can avoid hardcoding this, and derive the expected value from the test data?
There was a problem hiding this comment.
There's an easy solution to this, but doing so makes me a bit uncomfortable, since if we derive the date format we might introduce a bug that changes the formatter in both production and in the test. Also, hardcoded test data makes it very clear to anyone looking at this what date format is expected.
(There's another issue with this test where the test should be "equals" and not "contains" (since if the date also displayed the year, incorrectly, this test would still pass). I fixed that in the other PR I'm working on for making dates match the design.)
Are you okay with leaving it?
There was a problem hiding this comment.
I'm okay with leaving it. I see your point of making it obvious to anyone reading the test what the value should be, which is more obvious with the string than with a const.
packages/desktop/src/renderer/components/Channel/Channel.stories.tsx
Outdated
Show resolved
Hide resolved
| })} | ||
| pendingGeneralChannelRecreation={false} | ||
| removeFile={dummyRemoveFile} | ||
| {...args} |
There was a problem hiding this comment.
I don't think you're using the spread operator correctly here. I think you're trying to use the args partial you defined earlier, but you've ended up needing to hardcode a lot of the values included in that partial because you have put them next to args like
{...args}
otherVal='otherVal'
instead of using your hardcoded values to add new keys or replace keys in args like
{
...args,
otherVal: 'otherVal'
}
I think this would work better if you used the Template pattern used by the other stories.
There was a problem hiding this comment.
I think the reason we can't use the Template pattern here is because in this story for the interactivity we need component state (UseState) but Args in Storybook are static. (I don't totally understand this stuff yet.)
packages/desktop/src/renderer/components/Channel/Channel.stories.tsx
Outdated
Show resolved
Hide resolved
| groups: { [day: string]: DisplayableMessage[][] } | ||
| }>(mock_messages()) | ||
|
|
||
| const handleSend = (message: string) => { |
There was a problem hiding this comment.
The new message isn't ordered properly when sent in this interactive story. Also, I don't have a real Quiet instance with old messages, but in this interactive UI, scrolling above the top briefly shifts the floating date marker from Oct 26 to Today
Screen.Recording.2025-02-12.at.6.51.52.PM.mov
There was a problem hiding this comment.
Fixed ordering! I saw that and was being lazy because it didn't really affect what I was testing, but it's weird and definitely good to fix.
On the "Today" flashing on transitions, I was not able to reproduce what you were seeing in Storybook, so I assume the issue was happening in the app too and I just wasn't seeing it there either. I found what I think was the culprit and fixed it. Let me know if you still see it!
| ) | ||
| } | ||
|
|
||
| export { FETCHING_MESSAGES as fetchingChannelMessagesText, DELETING_CHANNEL as deletingChannelMessage } |
There was a problem hiding this comment.
Renaming constants in order to export them feels like an anti-pattern. We should just commit to renaming them with the all caps pattern everywhere. If you use F2 to rename a variable in your IDE, it should rename it everywhere automatically, so you don't have to do hacks like this to fix import problems.
| @@ -272,7 +272,7 @@ describe('publicChannelsSelectors', () => { | |||
|
|
|||
| // Get groups names | |||
| const groupDay1 = formatMessageDisplayDay(formatMessageDisplayDate(msgs['7'].createdAt)) | |||
| expect(groupDay1).toBe('Feb 05') | |||
| expect(groupDay1).toBe('Feb 5') | |||
There was a problem hiding this comment.
Another place we could be using references to the test data instead of hardcoding
There was a problem hiding this comment.
This is easy to fix but I have the same question/doubt as before.
…roper import for ModalName
Co-authored-by: Taea <[email protected]>
This addresses #505 to show sticky, floating date markers.
Pull Request Checklist