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

Jump to bottom

Add Swift evolution proposals dashboard #161

Merged
merged 8 commits into from Oct 31, 2022
Merged

Add Swift evolution proposals dashboard #161

merged 8 commits into from Oct 31, 2022

Conversation

kaishin
Copy link
Collaborator

@kaishin kaishin commented Oct 11, 2022 •
edited

This PR adds the Swift Evolution review dashboard and updates the content sections and navigation related to Swift Evolution across the site.

Motivation:

The following changes were proposed and discussed during the workgroup's meeting that took place on the 20th of September, 2022.

Modifications:

  • Move and update the the review dashboard code from the evolution repository. The new page lives under the "Open Source Development" section.
  • Changes to the original code include:
    • Drop reliance on JS for adding sticky support to the search bar. CSS support is at 96%.
    • Move color variables to the global scope and reuse pre-existing colors as much as possible. Review status colors were left intact and outside of the global color definition due to their usage being limited to the new page. We can explore unifying them at a later stage.
    • Minor JS refactors to improve readability.
    • Use a standard search field to benefit from browser features and simplify the code.
    • Simplify the layout of the metadata by forgoing a two-column grid in favor of a single column list. This was primarily done to keep layout styles simpler in anticipation of later layout refactors.

Preview:

Preview version is deployed here.

@kaishin kaishin marked this pull request as ready for review October 11, 2022 15:18
@shahmishal
Copy link
Member

cc @amartini51 for feedback

@amartini51
Copy link
Contributor

amartini51 commented Oct 11, 2022 •
edited

Also CC-ing @krilnon for feedback, since he worked on the JavaScript and web front-end parts of this, and knows more about those than I do.

@krilnon
Copy link
Member

krilnon commented Oct 11, 2022

Generally speaking, I think it's great to move this over to swift.org. This dashboard being hosted on GitHub Pages is mostly historical, since the website workgroup and open swift.org website repo didn't exist at the time.

At a glance, it looks like this preserves all the functionality and makes a good effort to fit in with the swift.org design. Nice work, @kaishin !

@tomerd
Copy link
Member

tomerd commented Oct 12, 2022

this is awesome! thanks putting together @kaishin

@tomerd
Copy link
Member

tomerd commented Oct 12, 2022

does it also support dark mode, as the rest of the website does?

@krilnon
Copy link
Member

krilnon commented Oct 12, 2022

does it also support dark mode, as the rest of the website does?

Yeah, added here originally: apple/swift-evolution#1177

@amartini51
Copy link
Contributor

@swift-ci Please test.

@amartini51
Copy link
Contributor

amartini51 commented Oct 12, 2022 •
edited

It looks like some fields might be wrapping to try and fit into a very narrow horizontal space. Is that something we can adjust?

For example, the implementation line on SE-0362 and the authors line SE-0327 could fit on one line, but they're wrapping:

image

image

@kaishin
Copy link
Collaborator Author

kaishin commented Oct 12, 2022

It looks like some fields might be wrapping to try and fit into a very narrow horizontal space. Is that something we can adjust?

@amartini51 Nice catch. Those were useful in the context of two-columns, but not anymore. Addressed in fe38fb7

@kaishin
Copy link
Collaborator Author

kaishin commented Oct 12, 2022

Preview updated with latest changes.

@amartini51
Copy link
Contributor

@amartini51 Nice catch. Those were useful in the context of two-columns, but not anymore. Addressed in fe38fb7

That looks much better, thanks!

amartini51
amartini51 approved these changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@amartini51 amartini51 left a comment

Choose a reason for hiding this comment

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

Reviewed the visual appearance and behavior (but not the HTML, JavaScript, or CSS) and that looks good to me.

benrimmington
benrimmington reviewed Oct 13, 2022
View reviewed changes
assets/javascripts/swift-evolution.js Show resolved Hide resolved
assets/javascripts/swift-evolution.js Outdated
Comment on lines 88 to 93
'.deferred': {
name: 'Deferred',
shortName: 'Deferred',
className: 'deferred',
count: 0
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this "Deferred" state be removed?
(And in the renderSearchBar and renderProposals functions.)

https://forums.swift.org/t/returning-or-rejecting-all-the-deferred-evolution-proposals/60724

@kaishin @benrimmington
Fix syntax typo
27dd26b 
Co-authored-by: Ben Rimmington <me@benrimmington.com>
@DougGregor
Copy link
Member

This looks fantastic!

shahmishal
shahmishal approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@shahmishal shahmishal left a comment

Choose a reason for hiding this comment

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

Thank you @kaishin!

@kaishin
Copy link
Collaborator Author

kaishin commented Oct 31, 2022

@swift-ci Please test

@kaishin kaishin merged commit eafe5d8 into apple:main Oct 31, 2022
@kaishin kaishin deleted the evolution-dashboard branch October 31, 2022 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomerd tomerd tomerd left review comments

@krilnon krilnon krilnon left review comments

@benrimmington benrimmington benrimmington left review comments

@shahmishal shahmishal shahmishal approved these changes

@amartini51 amartini51 amartini51 approved these changes

@0xTim 0xTim Awaiting requested review from 0xTim 0xTim is a code owner

@alexandersandberg alexandersandberg Awaiting requested review from alexandersandberg alexandersandberg is a code owner

@daveverwer daveverwer Awaiting requested review from daveverwer daveverwer is a code owner

@dempseyatgithub dempseyatgithub Awaiting requested review from dempseyatgithub dempseyatgithub is a code owner

@krstnfx krstnfx Awaiting requested review from krstnfx

@mschinis mschinis Awaiting requested review from mschinis

@TimTr TimTr Awaiting requested review from TimTr

@twostraws twostraws Awaiting requested review from twostraws

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@kaishin @shahmishal @amartini51 @krilnon @tomerd @DougGregor @benrimmington