Skip to content
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.

Sign up for GitHub

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

Jump to bottom

portico: Update screenshots for "for/X" pages . #30159

Merged
merged 7 commits into from
Jul 1, 2024
Merged

portico: Update screenshots for "for/X" pages . #30159

merged 7 commits into from
Jul 1, 2024

Conversation

roanster007
Copy link
Collaborator

@roanster007 roanster007 commented May 21, 2024 •
edited
Loading

This PR updates screenshots for the "for/X" pages mentioned in #30128.

Fixes #30128

Screenshots and screen captures:

/for/education image image
/for/events image image
/for/business or open-source image image
/for/research image image
Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@roanster007 roanster007 mentioned this pull request May 21, 2024
Closed
12 tasks
@roanster007 roanster007 changed the title portico: Update screenshots for "for/education" and "for/events" pages. portico: Update screenshots for "for/X" pages . May 22, 2024
@roanster007
Copy link
Collaborator Author

@alya updated all the screenshots (with avatars) mentioned in the issue.

@alya
Copy link
Contributor

alya commented Jun 3, 2024 •
edited
Loading

Thanks, and sorry for the slow review!

  1. The font size was already smaller than ideal, and is even tinier now. I think we should just make the screenshots narrower: use a narrow window, and end just to the right of the message content (cutting off the timestamps). It may also be helpful to zoom the window in.
  2. I'm not seeing the updated screenshots on /for/education when I test it, and there's one old one and one new one on /for/research and /for/events. Did you test this PR in dev?

@roanster007
Copy link
Collaborator Author

roanster007 commented Jun 3, 2024 •
edited
Loading

The images need to be manually updated in the respective pages since previous figma images combined images of 2 different topics as a single image. So, I had to keep the same image name/path, and add "_1" and "_2" suffixes to distinguish them.

@alya
Copy link
Contributor

alya commented Jun 3, 2024

I'm not sure what you mean the images being manually updated on the page. All the code for these pages is in the zulip/zulip repo, and is updated in PRs like everything else.

@roanster007
Copy link
Collaborator Author

@alya I updated the screenshots, narrowing the web page (to appear in similar width of existing screenshots), and updated the corresponding pages to reflect the changes.

@alya
Copy link
Contributor

alya commented Jun 5, 2024

Thanks! Please add screenshots of the resulting pages to the PR description.

@roanster007
Copy link
Collaborator Author

I guess we need to modify stream/topics names for some screenshots since they appear too long after narrowing.

@alya
Copy link
Contributor

alya commented Jun 7, 2024

Let's use the strategy we had before of cutting off the timestamps in the screenshots. They just kind of add noise in any case. Then we'll have more space for the channel/topic names.

@roanster007
Copy link
Collaborator Author

@alya I narrowed the window size, removed the timestamp column, and updated the pr and screenshots.

@alya
Copy link
Contributor

alya commented Jun 12, 2024

Thanks! Hm, I think these would look better if we could include the left edge. I think it won't cost that much horizontal space... Maybe try one page and post in #design, @-mentioning me? We should try to get this settled, without getting delayed by how often I work through updated PRs.

@roanster007 roanster007 force-pushed the iss-30128_2 branch 5 times, most recently from 9675e74 to 470effb Compare June 20, 2024 17:45
@andersk
Copy link
Member

andersk commented Jun 20, 2024

You’ve incorrectly changed the equation in events/message_formatting_day.png by adding a “+” that shouldn’t be there. Also, it was intended to demonstrate display math (```math), not inline math ($$).

@andersk
Copy link
Member

andersk commented Jun 20, 2024

Your code blocks in companies/message-formatting-top.png and education/message_formatting_day_1.png start with a leading space character that shouldn’t be there.

@andersk
Copy link
Member

andersk commented Jun 20, 2024 •
edited
Loading

education/interactive_messaging_day_2.png shows an unfortunate wrapping bug between “6:30 PM” and “!”. Part of this is #30515, but that doesn’t fix it by itself; we also need to avoid styling <time> with display: inline-block.

For now we can work around this by changing the message so it doesn’t wrap right before “!”.

@andersk
Copy link
Member

andersk commented Jun 20, 2024 •
edited
Loading

education/message_formatting_day_2.png shows some extra vertical space between Zoe’s name and her message. This seems to be a KaTeX CSS bug where .katex-mathml inflates the line-height even though it’s visually hidden in various ways.

For now we can work around this by changing the message so it doesn’t wrap right before the math.

@roanster007 roanster007 force-pushed the iss-30128_2 branch 2 times, most recently from de2a619 to 5aedb05 Compare June 21, 2024 21:09
@alya
Copy link
Contributor

alya commented Jun 21, 2024

Are we accidentally applying the right-side fading to the channels/topics image too? It doesn't make sense to do that on that one (which is not cut off in the same way, and has a border).
Screenshot 2024-06-21 at 14 22 34@2x

@alya
Copy link
Contributor

alya commented Jun 21, 2024

Looking close otherwise!

@roanster007
Copy link
Collaborator Author

roanster007 commented Jun 22, 2024 •
edited
Loading

Are we accidentally applying the right-side fading to the channels/topics image too? It doesn't make sense to do that on that one (which is not cut off in the same way, and has a border).

oops, I mixed up css class there. @alya fixed it.

image

@roanster007
Copy link
Collaborator Author

@timabbott this is ready for another review.

@roanster007
portico: Add user avatars directory.
a0addf6 
This commit adds a user avatars directory, with avatars for some
users, which could be used while generating screenshots.
@roanster007
portico: Extend the script to generate thread screenshots.
711b442 
This commit extends the script used to generate thread
screenshots to be able to create invite_only streams,
add starred messages, create user groups, add edited
notice to messages, and send messages from notification
bots.

The window width is updated, and background for the
screenshots is changed to white.

It is also updated so that user-avatars mapping
occurs within the script itself.

This is a prep commit to zulip#30128
@roanster007
css: Update css of landing page images to show fading effect.
29d75cf 
This commit adds the css class - "message-screenshot" and
"message-starred" to the "landing_page" to exhibit fading
effect.
@roanster007
portico: Update screenshots for "for/education" page.
08791af 
This commit adds the json file and updates screenshots
for the "for/education" page.

Fixes part of zulip#30128
@roanster007
portico: Update screenshots for "for/events" page.
e75de4c 
This commit adds the json file and updates screenshots
for the "for/events" page.

Fixes part of zulip#30128
@roanster007
portico: Update screenshots for "for/business", "for/open-source" pages.
83c14b7 
This commit updates screenshots for the "for/business" and
"for/open-source" pages, and adds the script for generating
their screenshots at "screenshots/companies.json."

Fixes part of zulip#30128
@roanster007
portico: Update screenshots for "for/research" page.
97f3560 
This commit adds the json file and updates screenshots
for the "for/research" page.

Fixes zulip#30128
@timabbott timabbott merged commit df68d3c into zulip:main Jul 1, 2024
14 checks passed
@timabbott
Copy link
Sponsor Member

Looks great, merged, thanks for all the work on this @roanster007!

@roanster007 roanster007 deleted the iss-30128_2 branch July 2, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timabbott timabbott timabbott left review comments

Assignees
No one assigned
Labels
size: XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update message content on /for/X pages
5 participants
@roanster007 @alya @andersk @timabbott @zulipbot