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.

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

Toggle button for file previews #3290

Merged
merged 19 commits into from
Jan 26, 2023
Merged

Toggle button for file previews #3290

merged 19 commits into from
Jan 26, 2023

Conversation

fiskus
Copy link
Member

@fiskus fiskus commented Jan 24, 2023

Screenshot

  • Changelog entry (skip if change is not significant to end users, e.g. docs only)

@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Merging #3290 (47bb37a) into master (d91e53b) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3290      +/-   ##
==========================================
- Coverage   35.30%   35.26%   -0.04%     
==========================================
  Files         666      668       +2     
  Lines       29234    29267      +33     
  Branches     4336     4340       +4     
==========================================
  Hits        10322    10322              
- Misses      17726    17759      +33     
  Partials     1186     1186              
Flag Coverage Δ
api-python 90.79% <ø> (ø)
catalog 8.86% <0.00%> (-0.02%) ⬇️
lambda 86.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
catalog/app/components/Preview/Menu.tsx 0.00% <0.00%> (ø)
catalog/app/components/Preview/ToggleButton.tsx 0.00% <0.00%> (ø)
catalog/app/components/Preview/index.js 0.00% <0.00%> (ø)
...alog/app/components/SearchResults/SearchResults.js 0.00% <0.00%> (ø)
catalog/app/containers/Bucket/Summarize.tsx 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fiskus fiskus requested a review from nl0 January 24, 2023 14:20
nl0
nl0 previously approved these changes Jan 24, 2023
Copy link
Member

@nl0 nl0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks fine, couple thoughts:

  • search results display is not affected by this change, is it?
  • i'd put the expand controls to the right, and hidden all the other stuff (e.g. download) into a "more" menu next to it -- this would imo work better for search results, bc search results show an entity type icon on the left (package vs object)

@fiskus
Copy link
Member Author

fiskus commented Jan 24, 2023

Hmmm...
Screenshot from 2023-01-24 23-15-42

@fiskus
Copy link
Member Author

fiskus commented Jan 24, 2023

Does it look ok for you? I'm not sure about my opinion yet

@fiskus
Copy link
Member Author

fiskus commented Jan 25, 2023

Screenshot from 2023-01-25 11-10-22

@fiskus fiskus changed the title Toggle button for file previews, auto-expand small previews Toggle button for file previews Jan 25, 2023
@fiskus
Copy link
Member Author

fiskus commented Jan 25, 2023

I put the expand/collapse button to the right, and shrank the download button into a 3-dots menu.
I moved these buttons to app/components/Preview directory and re-used them for SearchResults.

Also, I removed "auto-expand" functionality because it requires more work: some previews obtain height after a while (Jupyter, JsonDisplay). So, we need to listen resize etc., I don't want to pollute this PR with this.

@fiskus fiskus requested a review from nl0 January 25, 2023 12:56
@fiskus fiskus removed the request for review from nl0 January 25, 2023 15:16
@nl0
Copy link
Member

nl0 commented Jan 25, 2023

Does it look ok for you? I'm not sure about my opinion yet

that looked ok to me, tho the spacing btw the icon buttons might be a little tighter

@fiskus fiskus requested a review from nl0 January 25, 2023 18:01
Copy link
Member

@nl0 nl0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code seems fine, tho im not sold on the design -- full-fledged button seems too bulky for the place imo. but it's your call if you think it's better this way

@fiskus
Copy link
Member Author

fiskus commented Jan 26, 2023

I agree, that design looks nicer without EXPAND text and bordered button, but I'm afraid user couldn't find that button.

Let's let users get used to the new button position, then we can optimize the space and shrink this button.

@fiskus fiskus merged commit d425970 into master Jan 26, 2023
@fiskus fiskus deleted the expand-collapse-file-preview branch January 26, 2023 10:49
robnewman pushed a commit that referenced this pull request Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants