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

Rework Navigation #1168

Merged
merged 1 commit into from
Aug 11, 2022
Merged

Rework Navigation #1168

merged 1 commit into from
Aug 11, 2022

Conversation

Chartman123
Copy link
Collaborator

@Chartman123 Chartman123 commented Apr 13, 2022 •
edited
Loading

Closes #422 and closes #423

This will partially close the two issues: Sharing will be reworked so it won't be easy to classify. Showing the number of answers redundant in the sidebar and on the responses button won't make that much sense.

Signed-off-by: Christian Hartmann chris-hartmann@gmx.de

@Chartman123 Chartman123 added the 2. developing Work in progress label Apr 13, 2022
@Chartman123 Chartman123 self-assigned this Apr 13, 2022
@codecov
Copy link

codecov bot commented Apr 13, 2022 •
edited
Loading

Codecov Report

Merging #1168 (aa554dd) into master (44001e2) will increase coverage by 0.06%.
The diff coverage is 40.00%.

❗ Current head aa554dd differs from pull request most recent head 921273a. Consider uploading reports for the commit 921273a to get more accurate results

@@             Coverage Diff              @@
##             master    #1168      +/-   ##
============================================
+ Coverage     32.54%   32.60%   +0.06%     
- Complexity      474      479       +5     
============================================
  Files            46       46              
  Lines          1828     1843      +15     
============================================
+ Hits            595      601       +6     
- Misses         1233     1242       +9

@jotoeri

This comment was marked as outdated.

Sign in to view
@Chartman123

This comment was marked as outdated.

Sign in to view
@Chartman123 Chartman123 marked this pull request as ready for review April 17, 2022 19:12
@Chartman123 Chartman123 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 17, 2022
@jancborchardt
Copy link
Member

Looking very nice! :) Just for clarification, does the number counter show the responses since last viewing, or the total response count?

@Chartman123
Copy link
Collaborator Author

Chartman123 commented Apr 21, 2022 •
edited
Loading

does the number counter show the responses since last viewing, or the total response count?

@jancborchardt At the moment only the total. If we want to show a highlighted counter for new responses since the last visit, we would need to store the last visit timestamp and compare it to the submissions of the form.

@jancborchardt
Copy link
Member

Thanks @Chartman123, total count is fine, no need to overcomplicate it since we have notifications for new responses. :)

jotoeri
jotoeri approved these changes Apr 25, 2022
View reviewed changes
Copy link
Member

@jotoeri jotoeri left a comment

Choose a reason for hiding this comment

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

Small code-beauty would be good, but apart from that i'm happy with the current state! 🎉

src/components/AppNavigationForm.vue Outdated Show resolved Hide resolved
src/components/AppNavigationForm.vue Outdated Show resolved Hide resolved
@Chartman123 Chartman123 force-pushed the rework_navigation branch 3 times, most recently from 495a747 to 3ff3fb8 Compare April 25, 2022 20:45
skjnldsv
skjnldsv previously requested changes Apr 26, 2022
View reviewed changes
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Please fix the component instead

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 26, 2022
@jotoeri jotoeri added this to the 3.0 milestone May 1, 2022
@jotoeri

This comment was marked as outdated.

Sign in to view
@jotoeri

This comment was marked as outdated.

Sign in to view
@jotoeri

This comment was marked as outdated.

Sign in to view
@Chartman123

This comment was marked as outdated.

Sign in to view
@jotoeri

This comment was marked as outdated.

Sign in to view
@jotoeri
Copy link
Member

jotoeri commented Jul 29, 2022

With nextcloud-libraries/nextcloud-vue#2856 now looks like:
new

@jotoeri jotoeri requested review from skjnldsv and jancborchardt and removed request for jancborchardt July 29, 2022 08:48
@jotoeri jotoeri added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 29, 2022
@Chartman123 Chartman123 added pending fix Waiting for a fix on one of our dependencies design Related to the design labels Jul 29, 2022
@Chartman123 Chartman123 force-pushed the rework_navigation branch 2 times, most recently from 1b55934 to e4753a7 Compare August 4, 2022 14:13
@Chartman123 Chartman123 removed the pending fix Waiting for a fix on one of our dependencies label Aug 4, 2022
@Chartman123
Copy link
Collaborator Author

@jancborchardt @skjnldsv The Vue component has been made ready for this now. Could you please check again if we should adjust something here or if we can merge it? Thanks 🙃

jotoeri
jotoeri approved these changes Aug 11, 2022
View reviewed changes
Copy link
Member

@jotoeri jotoeri left a comment

Choose a reason for hiding this comment

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

Still fine by me... 👍 🙃

@Chartman123
Switch to ListItem and add counter/second line
921273a 
Signed-off-by: Christian Hartmann <chris-hartmann@gmx.de>
@Chartman123 Chartman123 merged commit f7d28d8 into master Aug 11, 2022
@Chartman123 Chartman123 deleted the rework_navigation branch August 11, 2022 17:00
@Chartman123 Chartman123 mentioned this pull request Sep 20, 2022
Closed
@jotoeri jotoeri added the enhancement New feature or request label Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jotoeri jotoeri jotoeri approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

Assignees

@Chartman123 Chartman123

Labels
3. to review Waiting for reviews design Related to the design enhancement New feature or request
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

Add navigation second line status Show number of responses on responses button
4 participants
@Chartman123 @jotoeri @jancborchardt @skjnldsv